From 418a62bb6ac72313cbe87b9519d4f5b87ffe6544 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 16 Jul 2024 18:11:54 +0200 Subject: [PATCH 1/5] JIT: Remove GTF_IND_INVARIANT and GTF_IND_NONFAULTING flags checking These flags are strictly optimizations. Having them required to be set for certain indirs based on context of the operand introduces IR invariants that are annoying to work with since it suddenly becomes necessary for all transformations to consider "did we just introduce this IR shape?". Instead, handle these patterns during morph as the optimization it is and remove the strict flags checking from `fgDebugCheckFlags`. --- src/coreclr/jit/fgdiagnostic.cpp | 30 ---------------------------- src/coreclr/jit/gentree.cpp | 34 +++++++++++++++++++++++++------- src/coreclr/jit/morph.cpp | 12 +++++++++-- 3 files changed, 37 insertions(+), 39 deletions(-) diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 45cdcfea0f46b6..4818d85f8381eb 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -3361,36 +3361,6 @@ void Compiler::fgDebugCheckFlags(GenTree* tree, BasicBlock* block) case GT_IND: // Do we have a constant integer address as op1 that is also a handle? - if (op1->IsIconHandle()) - { - if ((tree->gtFlags & GTF_IND_INVARIANT) != 0) - { - actualFlags |= GTF_IND_INVARIANT; - } - if ((tree->gtFlags & GTF_IND_NONFAULTING) != 0) - { - actualFlags |= GTF_IND_NONFAULTING; - } - - GenTreeFlags handleKind = op1->GetIconHandleFlag(); - - // Some of these aren't handles to invariant data... - if (GenTree::HandleKindDataIsInvariant(handleKind) && (handleKind != GTF_ICON_FTN_ADDR)) - { - expectedFlags |= GTF_IND_INVARIANT; - } - else - { - // For statics, we expect the GTF_GLOB_REF to be set. However, we currently - // fail to set it in a number of situations, and so this check is disabled. - // TODO: enable checking of GTF_GLOB_REF. - // expectedFlags |= GTF_GLOB_REF; - } - - // Currently we expect all indirections with constant addresses to be nonfaulting. - expectedFlags |= GTF_IND_NONFAULTING; - } - assert(((tree->gtFlags & GTF_IND_TGT_NOT_HEAP) == 0) || ((tree->gtFlags & GTF_IND_TGT_HEAP) == 0)); break; diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index f7da7729d23752..0b39412bc5334d 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -10631,8 +10631,7 @@ void GenTree::SetIndirExceptionFlags(Compiler* comp) //------------------------------------------------------------------------------ // HandleKindDataIsInvariant: Returns true if the data referred to by a handle -// address is guaranteed to be invariant. Note that GTF_ICON_FTN_ADDR handles may -// or may not point to invariant data. +// address is guaranteed to be invariant. // // Arguments: // flags - GenTree flags for handle. @@ -10643,11 +10642,32 @@ bool GenTree::HandleKindDataIsInvariant(GenTreeFlags flags) GenTreeFlags handleKind = flags & GTF_ICON_HDL_MASK; assert(handleKind != GTF_EMPTY); - // All handle types are assumed invariant except those specifically listed here. - - return (handleKind != GTF_ICON_STATIC_HDL) && // Pointer to a mutable class Static variable - (handleKind != GTF_ICON_BBC_PTR) && // Pointer to a mutable basic block count value - (handleKind != GTF_ICON_GLOBAL_PTR); // Pointer to mutable data from the VM state + switch (handleKind) + { + case GTF_ICON_SCOPE_HDL: + case GTF_ICON_CLASS_HDL: + case GTF_ICON_METHOD_HDL: + case GTF_ICON_FIELD_HDL: + case GTF_ICON_STR_HDL: + case GTF_ICON_CONST_PTR: + case GTF_ICON_VARG_HDL: + case GTF_ICON_PINVKI_HDL: + case GTF_ICON_TOKEN_HDL: + case GTF_ICON_TLS_HDL: + case GTF_ICON_CIDMID_HDL: + case GTF_ICON_FIELD_SEQ: + case GTF_ICON_STATIC_ADDR_PTR: + case GTF_ICON_SECREL_OFFSET: + case GTF_ICON_TLSGD_OFFSET: + return true; + case GTF_ICON_FTN_ADDR: + case GTF_ICON_GLOBAL_PTR: + case GTF_ICON_STATIC_HDL: + case GTF_ICON_BBC_PTR: + case GTF_ICON_STATIC_BOX_PTR: + default: + return false; + } } #ifdef DEBUG diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 8e510cdb99fc11..1045d96258bd61 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -8679,9 +8679,17 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA case GT_IND: { - if (op1->IsIconHandle(GTF_ICON_OBJ_HDL)) + if (op1->IsIconHandle()) { - tree->gtFlags |= (GTF_IND_INVARIANT | GTF_IND_NONFAULTING | GTF_IND_NONNULL); + // All indirections with (handle) constant addresses are + // nonfaulting. + tree->gtFlags |= GTF_IND_NONFAULTING; + + // We know some handle types always point to invariant data. + if (GenTree::HandleKindDataIsInvariant(op1->GetIconHandleFlag())) + { + tree->gtFlags |= GTF_IND_INVARIANT; + } } GenTree* optimizedTree = fgMorphFinalizeIndir(tree->AsIndir()); From 4572408c20a95e905b1e3a1b8623d5127f6db57f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 16 Jul 2024 18:16:50 +0200 Subject: [PATCH 2/5] Remove old comment --- src/coreclr/jit/fgdiagnostic.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 4818d85f8381eb..9f1a72afce63e4 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -3360,7 +3360,6 @@ void Compiler::fgDebugCheckFlags(GenTree* tree, BasicBlock* block) break; case GT_IND: - // Do we have a constant integer address as op1 that is also a handle? assert(((tree->gtFlags & GTF_IND_TGT_NOT_HEAP) == 0) || ((tree->gtFlags & GTF_IND_TGT_HEAP) == 0)); break; From 1af84b9e6196837563f03c4abbaa20dfa1f77ce0 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 16 Jul 2024 18:53:19 +0200 Subject: [PATCH 3/5] Remove another assert --- src/coreclr/jit/gentree.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 0b39412bc5334d..60ff8d61b65cd3 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -7571,8 +7571,6 @@ GenTree* Compiler::gtNewIndOfIconHandleNode(var_types indType, size_t addr, GenT if (isInvariant) { - assert(GenTree::HandleKindDataIsInvariant(iconFlags)); - // This indirection also is invariant. indirFlags |= GTF_IND_INVARIANT; From 629c793fb9620803ba49acca0a242cf1a39fa3a7 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 16 Jul 2024 19:34:55 +0200 Subject: [PATCH 4/5] Count --- src/coreclr/jit/gentree.h | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 757ce23912d732..c3eb11fbffa68e 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -513,18 +513,18 @@ enum GenTreeFlags : unsigned int GTF_ICON_FIELD_HDL = 0x04000000, // GT_CNS_INT -- constant is a field handle GTF_ICON_STATIC_HDL = 0x05000000, // GT_CNS_INT -- constant is a handle to static data GTF_ICON_STR_HDL = 0x06000000, // GT_CNS_INT -- constant is a pinned handle pointing to a string object - GTF_ICON_OBJ_HDL = 0x12000000, // GT_CNS_INT -- constant is an object handle (e.g. frozen string or Type object) - GTF_ICON_CONST_PTR = 0x07000000, // GT_CNS_INT -- constant is a pointer to immutable data, (e.g. IAT_PPVALUE) - GTF_ICON_GLOBAL_PTR = 0x08000000, // GT_CNS_INT -- constant is a pointer to mutable data (e.g. from the VM state) - GTF_ICON_VARG_HDL = 0x09000000, // GT_CNS_INT -- constant is a var arg cookie handle - GTF_ICON_PINVKI_HDL = 0x0A000000, // GT_CNS_INT -- constant is a pinvoke calli handle - GTF_ICON_TOKEN_HDL = 0x0B000000, // GT_CNS_INT -- constant is a token handle (other than class, method or field) - GTF_ICON_TLS_HDL = 0x0C000000, // GT_CNS_INT -- constant is a TLS ref with offset - GTF_ICON_FTN_ADDR = 0x0D000000, // GT_CNS_INT -- constant is a function address - GTF_ICON_CIDMID_HDL = 0x0E000000, // GT_CNS_INT -- constant is a class ID or a module ID - GTF_ICON_BBC_PTR = 0x0F000000, // GT_CNS_INT -- constant is a basic block count pointer - GTF_ICON_STATIC_BOX_PTR = 0x10000000, // GT_CNS_INT -- constant is an address of the box for a STATIC_IN_HEAP field - GTF_ICON_FIELD_SEQ = 0x11000000, // <--------> -- constant is a FieldSeq* (used only as VNHandle) + GTF_ICON_OBJ_HDL = 0x08000000, // GT_CNS_INT -- constant is an object handle (e.g. frozen string or Type object) + GTF_ICON_CONST_PTR = 0x08000000, // GT_CNS_INT -- constant is a pointer to immutable data, (e.g. IAT_PPVALUE) + GTF_ICON_GLOBAL_PTR = 0x09000000, // GT_CNS_INT -- constant is a pointer to mutable data (e.g. from the VM state) + GTF_ICON_VARG_HDL = 0x0A000000, // GT_CNS_INT -- constant is a var arg cookie handle + GTF_ICON_PINVKI_HDL = 0x0B000000, // GT_CNS_INT -- constant is a pinvoke calli handle + GTF_ICON_TOKEN_HDL = 0x0C000000, // GT_CNS_INT -- constant is a token handle (other than class, method or field) + GTF_ICON_TLS_HDL = 0x0D000000, // GT_CNS_INT -- constant is a TLS ref with offset + GTF_ICON_FTN_ADDR = 0x0E000000, // GT_CNS_INT -- constant is a function address + GTF_ICON_CIDMID_HDL = 0x0F000000, // GT_CNS_INT -- constant is a class ID or a module ID + GTF_ICON_BBC_PTR = 0x10000000, // GT_CNS_INT -- constant is a basic block count pointer + GTF_ICON_STATIC_BOX_PTR = 0x11000000, // GT_CNS_INT -- constant is an address of the box for a STATIC_IN_HEAP field + GTF_ICON_FIELD_SEQ = 0x12000000, // <--------> -- constant is a FieldSeq* (used only as VNHandle) GTF_ICON_STATIC_ADDR_PTR = 0x13000000, // GT_CNS_INT -- constant is a pointer to a static base address GTF_ICON_SECREL_OFFSET = 0x14000000, // GT_CNS_INT -- constant is an offset in a certain section. GTF_ICON_TLSGD_OFFSET = 0x15000000, // GT_CNS_INT -- constant is an argument to tls_get_addr. From b578203f3ab0e1dee81c8235c03612f3b2f9cac6 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 16 Jul 2024 19:35:26 +0200 Subject: [PATCH 5/5] Try 2 at counting --- src/coreclr/jit/gentree.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index c3eb11fbffa68e..94e88caedc7c7e 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -513,7 +513,7 @@ enum GenTreeFlags : unsigned int GTF_ICON_FIELD_HDL = 0x04000000, // GT_CNS_INT -- constant is a field handle GTF_ICON_STATIC_HDL = 0x05000000, // GT_CNS_INT -- constant is a handle to static data GTF_ICON_STR_HDL = 0x06000000, // GT_CNS_INT -- constant is a pinned handle pointing to a string object - GTF_ICON_OBJ_HDL = 0x08000000, // GT_CNS_INT -- constant is an object handle (e.g. frozen string or Type object) + GTF_ICON_OBJ_HDL = 0x07000000, // GT_CNS_INT -- constant is an object handle (e.g. frozen string or Type object) GTF_ICON_CONST_PTR = 0x08000000, // GT_CNS_INT -- constant is a pointer to immutable data, (e.g. IAT_PPVALUE) GTF_ICON_GLOBAL_PTR = 0x09000000, // GT_CNS_INT -- constant is a pointer to mutable data (e.g. from the VM state) GTF_ICON_VARG_HDL = 0x0A000000, // GT_CNS_INT -- constant is a var arg cookie handle