From b3be592113f771fe3f290afae9cde2676996ce89 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 27 Oct 2022 22:07:01 +0300 Subject: [PATCH 1/9] Note struct field access --- src/coreclr/jit/gentree.cpp | 23 ++++++++++++++++++++--- src/coreclr/jit/gentree.h | 1 + 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 24081df5c38aeb..7c7bff2c4d463c 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -7584,10 +7584,27 @@ GenTreeField* Compiler::gtNewFieldAddrNode(var_types type, CORINFO_FIELD_HANDLE GenTreeField* fieldNode = new (this, GT_FIELD_ADDR) GenTreeField(GT_FIELD_ADDR, type, obj, fldHnd, offset); - // TODO-ADDR: add GTF_EXCEPT handling here and delete it from callers. - // TODO-ADDR: delete this zero-diff quirk. - fieldNode->gtFlags |= GTF_GLOB_REF; + // If "obj" is the address of a local, note that a field of that struct local has been accessed. + if ((obj != nullptr) && obj->OperIs(GT_ADDR) && varTypeIsStruct(obj->AsUnOp()->gtOp1) && + obj->AsUnOp()->gtOp1->OperIs(GT_LCL_VAR)) + { + LclVarDsc* varDsc = lvaGetDesc(obj->AsUnOp()->gtOp1->AsLclVarCommon()); + + varDsc->lvFieldAccessed = 1; + if (lvaIsImplicitByRefLocal(lvaGetLclNum(varDsc))) + { + // TODO-ADDR: delete this zero-diff quirk. + fieldNode->gtFlags |= GTF_GLOB_REF; + } + } + else + { + // TODO-ADDR: delete this zero-diff quirk. + fieldNode->gtFlags |= GTF_GLOB_REF; + } + + // TODO-ADDR: add GTF_EXCEPT handling here and delete it from callers. return fieldNode; } diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index b78096914ebab2..7a7558eda71595 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -4053,6 +4053,7 @@ struct GenTreeField : public GenTreeUnOp // True if this field is a volatile memory operation. bool IsVolatile() const { + assert(((gtFlags & GTF_FLD_VOLATILE) == 0) || OperIs(GT_FIELD)); return (gtFlags & GTF_FLD_VOLATILE) != 0; } From 0170e2fd98b4a9ce6230570b620c6f18b780a372 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Mon, 4 Jul 2022 00:24:38 +0300 Subject: [PATCH 2/9] FIELD_ADDR in local morph --- src/coreclr/jit/lclmorph.cpp | 80 +++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 32 deletions(-) diff --git a/src/coreclr/jit/lclmorph.cpp b/src/coreclr/jit/lclmorph.cpp index c1d57adf58208a..f87a94c8dc7a83 100644 --- a/src/coreclr/jit/lclmorph.cpp +++ b/src/coreclr/jit/lclmorph.cpp @@ -192,28 +192,26 @@ class LocalAddressVisitor final : public GenTreeVisitor } //------------------------------------------------------------------------ - // Field: Produce a location value from an address value. + // AddOffset: Produce an address value from an address value. // // Arguments: - // val - the input value - // field - the FIELD node that uses the input address value - // compiler - the compiler instance + // val - the input value + // addOffset - the offset to add // // Return Value: // `true` if the value was consumed. `false` if the input value - // cannot be consumed because it is itself a location or because + // cannot be consumed because it is not an address or because // the offset overflowed. In this case the caller is expected // to escape the input value. // // Notes: // - LOCATION(lclNum, offset) => not representable, must escape - // - ADDRESS(lclNum, offset) => LOCATION(lclNum, offset + field.Offset) - // if the offset overflows then location is not representable, must escape + // - ADDRESS(lclNum, offset) => ADDRESS(lclNum, offset + offs) // - UNKNOWN => UNKNOWN // - bool Field(Value& val, GenTreeField* field, Compiler* compiler) + bool AddOffset(Value& val, unsigned addOffset) { - assert(!IsLocation() && !IsAddress()); + assert(!IsAddress() && !IsLocation()); if (val.IsLocation()) { @@ -222,16 +220,16 @@ class LocalAddressVisitor final : public GenTreeVisitor if (val.IsAddress()) { - ClrSafeInt newOffset = - ClrSafeInt(val.m_offset) + ClrSafeInt(field->gtFldOffset); + ClrSafeInt newOffset = ClrSafeInt(val.m_offset) + ClrSafeInt(addOffset); if (newOffset.IsOverflow()) { return false; } - m_lclNum = val.m_lclNum; - m_offset = newOffset.Value(); + m_lclNum = val.m_lclNum; + m_offset = newOffset.Value(); + m_address = true; } INDEBUG(val.Consume();) @@ -242,35 +240,33 @@ class LocalAddressVisitor final : public GenTreeVisitor // Indir: Produce a location value from an address value. // // Arguments: - // val - the input value + // val - the input value + // addOffset - the offset to add + // field - the FIELD node that uses the input address value // // Return Value: // `true` if the value was consumed. `false` if the input value - // cannot be consumed because it is itself a location. In this - // case the caller is expected to escape the input value. + // cannot be consumed because it is itsef a location or because + // the offset overflowed. In this case the caller is expected + // to escape the input value. // // Notes: // - LOCATION(lclNum, offset) => not representable, must escape - // - ADDRESS(lclNum, offset) => LOCATION(lclNum, offset) + // - ADDRESS(lclNum, offset) => LOCATION(lclNum, offset + addOffset) + // if the offset overflows then location is not representable, must escape // - UNKNOWN => UNKNOWN // - bool Indir(Value& val) + bool Indir(Value& val, unsigned addOffset = 0) { assert(!IsLocation() && !IsAddress()); - if (val.IsLocation()) - { - return false; - } - - if (val.IsAddress()) + if (AddOffset(val, addOffset)) { - m_lclNum = val.m_lclNum; - m_offset = val.m_offset; + m_address = false; + return true; } - INDEBUG(val.Consume();) - return true; + return false; } #ifdef DEBUG @@ -464,8 +460,28 @@ class LocalAddressVisitor final : public GenTreeVisitor PopValue(); break; + case GT_FIELD_ADDR: + if (node->AsField()->IsInstance()) + { + assert(TopValue(1).Node() == node); + assert(TopValue(0).Node() == node->AsField()->GetFldObj()); + + if (!TopValue(1).AddOffset(TopValue(0), node->AsField()->gtFldOffset)) + { + // The field object did not represent an address, or the latter overflowed. + EscapeValue(TopValue(0), node); + } + + PopValue(); + } + else + { + assert(TopValue(0).Node() == node); + } + break; + case GT_FIELD: - if (node->AsField()->GetFldObj() != nullptr) + if (node->AsField()->IsInstance()) { assert(TopValue(1).Node() == node); assert(TopValue(0).Node() == node->AsField()->GetFldObj()); @@ -475,10 +491,10 @@ class LocalAddressVisitor final : public GenTreeVisitor // Volatile indirections must not be removed so the address, if any, must be escaped. EscapeValue(TopValue(0), node); } - else if (!TopValue(1).Field(TopValue(0), node->AsField(), m_compiler)) + else if (!TopValue(1).Indir(TopValue(0), node->AsField()->gtFldOffset)) { - // Either the address comes from a location value (e.g. FIELD(IND(...))) - // or the field offset has overflowed. + // Either the address comes from a location value (e.g. FIELD(IND(...))) or the field + // offset has overflowed. EscapeValue(TopValue(0), node); } From 40b1cbf16c663f6a2afa7341d9d016f1b2e14a13 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 27 Oct 2022 22:20:59 +0300 Subject: [PATCH 3/9] Enable in importer --- src/coreclr/jit/importer.cpp | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 19e2c842393b54..ee4a49aaff3999 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -9522,11 +9522,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) obj = impGetStructAddr(obj, objType, CHECK_SPILL_ALL, true); } - DWORD typeFlags = info.compCompHnd->getClassAttribs(resolvedToken.hClass); - - // TODO-ADDR: use FIELD_ADDR for all fields, not just those of classes. - // - if (isLoadAddress && ((typeFlags & CORINFO_FLG_VALUECLASS) == 0)) + if (isLoadAddress) { op1 = gtNewFieldAddrNode(varTypeIsGC(obj) ? TYP_BYREF : TYP_I_IMPL, resolvedToken.hField, obj, fieldInfo.offset); @@ -9548,17 +9544,11 @@ void Compiler::impImportBlockCode(BasicBlock* block) op1->gtFlags |= GTF_EXCEPT; } - if (StructHasOverlappingFields(typeFlags)) + if (StructHasOverlappingFields(info.compCompHnd->getClassAttribs(resolvedToken.hClass))) { op1->AsField()->gtFldMayOverlap = true; } - // Wrap it in a address of operator if necessary. - if (isLoadAddress && op1->OperIs(GT_FIELD)) - { - op1 = gtNewOperNode(GT_ADDR, varTypeIsGC(obj) ? TYP_BYREF : TYP_I_IMPL, op1); - } - if (!isLoadAddress && compIsForInlining() && impInlineIsGuaranteedThisDerefBeforeAnySideEffects(nullptr, nullptr, obj, impInlineInfo->inlArgInfo)) From f0f96d1b6a773d9c11267bd32086754bcafe4b35 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 27 Oct 2022 22:26:18 +0300 Subject: [PATCH 4/9] Fix impIsAddressInLocal --- src/coreclr/jit/importer.cpp | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index ee4a49aaff3999..37cc88bd488d28 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -12918,41 +12918,27 @@ bool Compiler::impIsInvariant(const GenTree* tree) // // Remarks: // This is a variant of GenTree::IsLocalAddrExpr that is more suitable for -// use during import. Unlike that function, this one handles GT_FIELD nodes. +// use during import. Unlike that function, this one handles field nodes. // bool Compiler::impIsAddressInLocal(const GenTree* tree, GenTree** lclVarTreeOut) { - if (tree->gtOper != GT_ADDR) - { - return false; - } - - GenTree* op = tree->AsOp()->gtOp1; - while (op->gtOper == GT_FIELD) + const GenTree* op = tree; + while (op->OperIs(GT_FIELD_ADDR) && op->AsField()->IsInstance()) { op = op->AsField()->GetFldObj(); - if (op && op->gtOper == GT_ADDR) // Skip static fields where op will be NULL. - { - op = op->AsOp()->gtOp1; - } - else - { - return false; - } } - if (op->gtOper == GT_LCL_VAR) + if (op->OperIs(GT_ADDR) && op->AsUnOp()->gtOp1->OperIs(GT_LCL_VAR)) { if (lclVarTreeOut != nullptr) { - *lclVarTreeOut = op; + *lclVarTreeOut = op->AsUnOp()->gtOp1; } + return true; } - else - { - return false; - } + + return false; } //------------------------------------------------------------------------ From 043457807f7fc5360037ae9c8d8c239358e4c1c2 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 27 Oct 2022 22:30:34 +0300 Subject: [PATCH 5/9] Add a TODO-ADDR note --- src/coreclr/jit/gentree.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 7c7bff2c4d463c..af59c6f64ed60d 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -7567,7 +7567,11 @@ GenTreeField* Compiler::gtNewFieldRef(var_types type, CORINFO_FIELD_HANDLE fldHn } //------------------------------------------------------------------------ -// gtNewFieldRef: Create a new GT_FIELD_ADDR node. +// gtNewFieldAddrNode: Create a new GT_FIELD_ADDR node. +// +// TODO-ADDR: consider creating a variant of this which would skip various +// no-op constructs (such as struct fields with zero offsets), and fold +// others (LCL_VAR_ADDR + FIELD_ADDR => LCL_FLD_ADDR). // // Arguments: // type - type for the address node From f51ab8ed02cd4c30536ac95bd1b57c10d6966649 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 27 Oct 2022 23:30:59 +0300 Subject: [PATCH 6/9] Local morph: promotion --- src/coreclr/jit/lclmorph.cpp | 47 +++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/src/coreclr/jit/lclmorph.cpp b/src/coreclr/jit/lclmorph.cpp index f87a94c8dc7a83..b1309b17685868 100644 --- a/src/coreclr/jit/lclmorph.cpp +++ b/src/coreclr/jit/lclmorph.cpp @@ -377,7 +377,7 @@ class LocalAddressVisitor final : public GenTreeVisitor { GenTree* const node = *use; - if (node->OperIs(GT_IND, GT_FIELD)) + if (node->OperIs(GT_IND, GT_FIELD, GT_FIELD_ADDR)) { MorphStructField(node, user); } @@ -1094,18 +1094,18 @@ class LocalAddressVisitor final : public GenTreeVisitor // FIELD(ADDR(LCL_VAR))) to a GT_LCL_VAR that references the struct field. // // Arguments: - // indir - the GT_IND/GT_FIELD node - // user - the node that uses the field + // node - the GT_IND/GT_FIELD/GT_FIELD_ADDR node + // user - the node that uses the field // // Notes: // This does not do anything if the access does not denote a promoted // struct field. // - void MorphStructField(GenTree* indir, GenTree* user) + void MorphStructField(GenTree* node, GenTree* user) { - assert(indir->OperIs(GT_IND, GT_FIELD)); + assert(node->OperIs(GT_IND, GT_FIELD, GT_FIELD_ADDR)); - GenTree* objRef = indir->AsUnOp()->gtOp1; + GenTree* objRef = node->AsUnOp()->gtOp1; GenTree* obj = ((objRef != nullptr) && objRef->OperIs(GT_ADDR)) ? objRef->AsOp()->gtOp1 : nullptr; // TODO-Bug: this code does not pay attention to "GTF_IND_VOLATILE". @@ -1115,7 +1115,7 @@ class LocalAddressVisitor final : public GenTreeVisitor if (varDsc->lvPromoted) { - unsigned fieldOffset = indir->OperIs(GT_FIELD) ? indir->AsField()->gtFldOffset : 0; + unsigned fieldOffset = node->OperIs(GT_IND) ? 0 : node->AsField()->gtFldOffset; unsigned fieldLclNum = m_compiler->lvaGetFieldLocal(varDsc, fieldOffset); if (fieldLclNum == BAD_VAR_NUM) @@ -1128,16 +1128,21 @@ class LocalAddressVisitor final : public GenTreeVisitor const LclVarDsc* fieldDsc = m_compiler->lvaGetDesc(fieldLclNum); var_types fieldType = fieldDsc->TypeGet(); GenTree* lclVarNode = nullptr; - GenTreeFlags lclVarFlags = indir->gtFlags & (GTF_NODE_MASK | GTF_DONT_CSE); + GenTreeFlags lclVarFlags = node->gtFlags & (GTF_NODE_MASK | GTF_DONT_CSE); + assert(fieldType != TYP_STRUCT); // Promoted LCL_VAR can't have a struct type. - assert(fieldType != TYP_STRUCT); // promoted LCL_VAR can't have a struct type. - if ((indir->TypeGet() == fieldType) || ((user != nullptr) && user->OperIs(GT_ADDR))) + if (node->OperIs(GT_FIELD_ADDR)) { - lclVarNode = indir; + node->ChangeOper(GT_ADDR); + node->AsUnOp()->gtOp1 = obj; + lclVarNode = obj; + } + else if ((node->TypeGet() == fieldType) || ((user != nullptr) && user->OperIs(GT_ADDR))) + { if (user != nullptr) { - if (user->OperIs(GT_ASG) && (user->AsOp()->gtOp1 == indir)) + if (user->OperIs(GT_ASG) && (user->AsOp()->gtOp1 == node)) { lclVarFlags |= GTF_VAR_DEF; } @@ -1147,6 +1152,8 @@ class LocalAddressVisitor final : public GenTreeVisitor lclVarFlags &= ~GTF_DONT_CSE; } } + + lclVarNode = node; } else // Here we will turn "FIELD/IND(ADDR(LCL_VAR))" into "OBJ/IND(ADDR(LCL_VAR))". { @@ -1155,26 +1162,26 @@ class LocalAddressVisitor final : public GenTreeVisitor // the promoted local would look like "{ int a, B }", while the IR would contain "FIELD" // nodes for the outer struct "A". // - if (indir->TypeIs(TYP_STRUCT)) + if (node->TypeIs(TYP_STRUCT)) { // TODO-1stClassStructs: delete this once "IND" nodes are no more. - if (indir->OperIs(GT_IND)) + if (node->OperIs(GT_IND)) { // We do not have a layout for this node. return; } - ClassLayout* layout = indir->GetLayout(m_compiler); - indir->SetOper(GT_OBJ); - indir->AsBlk()->SetLayout(layout); - indir->AsBlk()->gtBlkOpKind = GenTreeBlk::BlkOpKindInvalid; + ClassLayout* layout = node->GetLayout(m_compiler); + node->SetOper(GT_OBJ); + node->AsBlk()->SetLayout(layout); + node->AsBlk()->gtBlkOpKind = GenTreeBlk::BlkOpKindInvalid; #ifndef JIT32_GCENCODER - indir->AsBlk()->gtBlkOpGcUnsafe = false; + node->AsBlk()->gtBlkOpGcUnsafe = false; #endif // !JIT32_GCENCODER } else { - indir->SetOper(GT_IND); + node->SetOper(GT_IND); } lclVarNode = obj; From 3a8661412a4e7c5f93b19d37fbe4b2e8e1d3d188 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sat, 29 Oct 2022 23:16:20 +0300 Subject: [PATCH 7/9] Fix up morph's address context --- src/coreclr/jit/compiler.h | 15 +--- src/coreclr/jit/morph.cpp | 153 +++++++------------------------------ 2 files changed, 29 insertions(+), 139 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index a1b860ed99c2e4..f523d22828b82e 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5677,22 +5677,9 @@ class Compiler // know will be dereferenced. To know that reliance on implicit null checking is sound, we must further know that // all offsets between the top-level indirection and the bottom are constant, and that their sum is sufficiently // small; hence the other fields of MorphAddrContext. - enum MorphAddrContextKind - { - MACK_Ind, - MACK_Addr, - }; struct MorphAddrContext { - MorphAddrContextKind m_kind; - bool m_allConstantOffsets; // Valid only for "m_kind == MACK_Ind". True iff all offsets between - // top-level indirection and here have been constants. - size_t m_totalOffset; // Valid only for "m_kind == MACK_Ind", and if "m_allConstantOffsets" is true. - // In that case, is the sum of those constant offsets. - - MorphAddrContext(MorphAddrContextKind kind) : m_kind(kind), m_allConstantOffsets(true), m_totalOffset(0) - { - } + size_t m_totalOffset = 0; // Sum of offsets between the top-level indirection and here (current context). }; #ifdef FEATURE_SIMD diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 16b28efe45a01d..5ccb081c156cb5 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -4990,7 +4990,7 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) #ifdef FEATURE_SIMD // if this field belongs to simd struct, translate it to simd intrinsic. - if ((mac == nullptr) && tree->OperIs(GT_FIELD)) + if (tree->OperIs(GT_FIELD)) { if (IsBaselineSimdIsaSupported()) { @@ -5012,19 +5012,10 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) } #endif - MorphAddrContext indMAC(MACK_Ind); - MorphAddrContext addrMAC(MACK_Addr); - bool isAddr = tree->OperIs(GT_FIELD_ADDR); + bool isAddr = tree->OperIs(GT_FIELD_ADDR); if (fieldNode->IsInstance()) { - // NULL mac means we encounter the GT_FIELD/GT_FIELD_ADDR first (and don't know our parent). - if (mac == nullptr) - { - // FIELD denotes a dereference of the field, equivalent to a MACK_Ind with zero offset. - mac = tree->OperIs(GT_FIELD) ? &indMAC : &addrMAC; - } - tree = fgMorphExpandInstanceField(tree, mac); } else if (fieldNode->IsTlsStatic()) @@ -5156,13 +5147,15 @@ GenTree* Compiler::fgMorphExpandInstanceField(GenTree* tree, MorphAddrContext* m if (fgAddrCouldBeNull(objRef)) { - if (!mac->m_allConstantOffsets || fgIsBigOffset(mac->m_totalOffset + fieldOffset)) + if (tree->OperIs(GT_FIELD)) { - addExplicitNullCheck = true; + addExplicitNullCheck = fgIsBigOffset(fieldOffset); } else { - addExplicitNullCheck = mac->m_kind == MACK_Addr; + // A non-null context here implies our [+ some offset] parent is an indirection, one that + // will implicitly null-check the produced address. + addExplicitNullCheck = (mac == nullptr) || fgIsBigOffset(mac->m_totalOffset + fieldOffset); } } @@ -9376,6 +9369,11 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA break; case GT_ADDR: + if (op1->OperIs(GT_FIELD) && op1->AsField()->IsInstance()) + { + op1->SetOper(GT_FIELD_ADDR); + return fgMorphField(op1, mac); + } /* op1 of a GT_ADDR is an l-value. Only r-values can be CSEed */ op1->gtFlags |= GTF_DONT_CSE; @@ -9920,93 +9918,33 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA } } - // We might need a new MorphAddressContext context. (These are used to convey - // parent context about how addresses being calculated will be used; see the - // specification comment for MorphAddrContext for full details.) - // Assume it's an Ind context to start. - MorphAddrContext subIndMac1(MACK_Ind); - MorphAddrContext* subMac1 = mac; - if (subMac1 == nullptr || subMac1->m_kind == MACK_Ind) + MorphAddrContext indMac; + if (tree->OperIsIndir()) // TODO-CQ: add more operators here (e. g. atomics). { - switch (tree->gtOper) - { - case GT_ADDR: - // A non-null mac here implies this node is part of an address computation. - // If so, we need to pass the existing mac down to the child node. - // - // Otherwise, use a new mac. - if (subMac1 == nullptr) - { - subMac1 = &subIndMac1; - subMac1->m_kind = MACK_Addr; - } - break; - case GT_COMMA: - // In a comma, the incoming context only applies to the rightmost arg of the - // comma list. The left arg (op1) gets a fresh context. - subMac1 = nullptr; - break; - case GT_OBJ: - case GT_BLK: - case GT_IND: - // A non-null mac here implies this node is part of an address computation (the tree parent is - // GT_ADDR). - // If so, we need to pass the existing mac down to the child node. - // - // Otherwise, use a new mac. - if (subMac1 == nullptr) - { - subMac1 = &subIndMac1; - } - break; - default: - break; - } + // Communicate to address morphing that the parent is an indirection. + mac = &indMac; } - - // For additions, if we're in an IND context keep track of whether - // all offsets added to the address are constant, and their sum. - if (tree->gtOper == GT_ADD && subMac1 != nullptr) + // For additions, if we already have a context, keep track of whether all offsets added + // to the address are constant, and their sum does not overflow. + else if ((mac != nullptr) && tree->OperIs(GT_ADD) && op2->IsCnsIntOrI()) { - assert(subMac1->m_kind == MACK_Ind || subMac1->m_kind == MACK_Addr); // Can't be a CopyBlock. - GenTree* otherOp = tree->AsOp()->gtOp2; - // Is the other operator a constant? - if (otherOp->IsCnsIntOrI()) + ClrSafeInt offset(mac->m_totalOffset); + offset += op2->AsIntCon()->IconValue(); + if (!offset.IsOverflow()) { - ClrSafeInt totalOffset(subMac1->m_totalOffset); - totalOffset += otherOp->AsIntConCommon()->IconValue(); - if (totalOffset.IsOverflow()) - { - // We will consider an offset so large as to overflow as "not a constant" -- - // we will do a null check. - subMac1->m_allConstantOffsets = false; - } - else - { - subMac1->m_totalOffset += otherOp->AsIntConCommon()->IconValue(); - } + mac->m_totalOffset = offset.Value(); } else { - subMac1->m_allConstantOffsets = false; + mac = nullptr; } } - - // If op1 is a GT_FIELD or indir, we need to pass down the mac if - // its parent is GT_ADDR, since the address of op1 - // is part of an ongoing address computation. Otherwise - // op1 represents the value of the field and so any address - // calculations it does are in a new context. - if (((op1->gtOper == GT_FIELD) || op1->OperIsIndir()) && (tree->gtOper != GT_ADDR)) + else // Reset the context. { - subMac1 = nullptr; - - // The impact of op1's value to any ongoing - // address computation is handled below when looking - // at op2. + mac = nullptr; } - tree->AsOp()->gtOp1 = op1 = fgMorphTree(op1, subMac1); + tree->AsOp()->gtOp1 = op1 = fgMorphTree(op1, mac); // If we are exiting the "then" part of a Qmark-Colon we must // save the state of the current copy assignment table @@ -10074,42 +10012,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA } } - // We might need a new MorphAddressContext context to use in evaluating op2. - // (These are used to convey parent context about how addresses being calculated - // will be used; see the specification comment for MorphAddrContext for full details.) - // Assume it's an Ind context to start. - switch (tree->gtOper) - { - case GT_ADD: - if (mac != nullptr && mac->m_kind == MACK_Ind) - { - GenTree* otherOp = tree->AsOp()->gtOp1; - // Is the other operator a constant? - if (otherOp->IsCnsIntOrI()) - { - mac->m_totalOffset += otherOp->AsIntConCommon()->IconValue(); - } - else - { - mac->m_allConstantOffsets = false; - } - } - break; - default: - break; - } - - // If op2 is a GT_FIELD or indir, we must be taking its value, - // so it should evaluate its address in a new context. - if ((op2->gtOper == GT_FIELD) || op2->OperIsIndir()) - { - // The impact of op2's value to any ongoing - // address computation is handled above when looking - // at op1. - mac = nullptr; - } - - tree->AsOp()->gtOp2 = op2 = fgMorphTree(op2, mac); + tree->AsOp()->gtOp2 = op2 = fgMorphTree(op2); /* Propagate the side effect flags from op2 */ From a82cce8c9cc491eab229570bd54a821c9ec4f274 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Fri, 11 Nov 2022 22:35:40 +0300 Subject: [PATCH 8/9] Add a test for #77636 --- .../JitBlue/Runtime_77636/Runtime_77636.cs | 35 +++++++++++++++++++ .../Runtime_77636/Runtime_77636.csproj | 10 ++++++ 2 files changed, 45 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_77636/Runtime_77636.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_77636/Runtime_77636.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_77636/Runtime_77636.cs b/src/tests/JIT/Regression/JitBlue/Runtime_77636/Runtime_77636.cs new file mode 100644 index 00000000000000..fb5c759419d836 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_77636/Runtime_77636.cs @@ -0,0 +1,35 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; + +unsafe class Runtime_77636 +{ + public static int Main() + { + try + { + Problem(null); + } + catch (NullReferenceException) + { + return 100; + } + + return 101; + } + + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int Problem(StructWithIndex* s) + { + return *(int*)((nint)(int*)&s->Value | (-1 & ~1)); + } + + struct StructWithIndex + { + public int Index; + public int Value; + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_77636/Runtime_77636.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_77636/Runtime_77636.csproj new file mode 100644 index 00000000000000..54ecf886782d7b --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_77636/Runtime_77636.csproj @@ -0,0 +1,10 @@ + + + Exe + True + true + + + + + From 57130685efaadbeffb29eb0d417934938210fb48 Mon Sep 17 00:00:00 2001 From: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> Date: Mon, 28 Nov 2022 22:58:04 +0300 Subject: [PATCH 9/9] Fix find&replace error --- src/coreclr/jit/lclmorph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lclmorph.cpp b/src/coreclr/jit/lclmorph.cpp index 0542432c8f6277..4a9961f017d297 100644 --- a/src/coreclr/jit/lclmorph.cpp +++ b/src/coreclr/jit/lclmorph.cpp @@ -1216,7 +1216,7 @@ class LocalAddressVisitor final : public GenTreeVisitor unsigned indSize = node->TypeIs(TYP_STRUCT) ? layout->GetSize() : genTypeSize(node); if (indSize > genTypeSize(fieldType)) { - // Retargeting this nodeection to reference the promoted field would make it + // Retargeting this indirection to reference the promoted field would make it // "wide", address-exposing the whole parent struct (with all of its fields). return; }