From d37a9b874f0e0dfd30110704de603e63dbe3be9f Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 28 Aug 2022 00:47:19 +0300 Subject: [PATCH 1/4] Add field offsets to array addresses This allows us to re-associate the expessions such that the outer field offset additions can get folded into: ADD ARR_ADDR T[] ADD LCL_VAR array CNS_INT 40 8 => ARR_ADDR T[] [+8] ADD ADD LCL_VAR array CNS_INT 40 8 => ARR_ADDR T[] [+8] ADD LCL_VAR array CNS_INT 48 --- src/coreclr/jit/gentree.cpp | 18 ++++++++++-------- src/coreclr/jit/gentree.h | 31 ++++++++++++++++++++++++++++++- src/coreclr/jit/morph.cpp | 24 +++++++++++++++++++++++- 3 files changed, 63 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 3b7c12f25ccb12..9b322681e1a5ad 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -8495,7 +8495,8 @@ GenTree* Compiler::gtCloneExpr( case GT_ARR_ADDR: copy = new (this, GT_ARR_ADDR) GenTreeArrAddr(tree->AsArrAddr()->Addr(), tree->AsArrAddr()->GetElemType(), - tree->AsArrAddr()->GetElemClassHandle(), tree->AsArrAddr()->GetFirstElemOffset()); + tree->AsArrAddr()->GetElemClassHandle(), tree->AsArrAddr()->GetElemSize(), + tree->AsArrAddr()->GetFirstElemOffset(), tree->AsArrAddr()->GetFieldOffset()); break; case GT_ARR_LENGTH: @@ -10630,6 +10631,11 @@ void Compiler::gtDispNode(GenTree* tree, IndentStack* indentStack, _In_ _In_opt_ { printf("%s[]", varTypeName(elemType)); } + + if (tree->OperIs(GT_ARR_ADDR) && (tree->AsArrAddr()->GetFieldOffset() != 0)) + { + printf(" [+%u]", tree->AsArrAddr()->GetFieldOffset()); + } } if (tree->OperIsLocal()) @@ -18128,13 +18134,9 @@ void GenTreeArrAddr::ParseArrayAddress(Compiler* comp, GenTree** pArr, ValueNum* } // OK, new we have to figure out if any part of the "offset" is a constant contribution to the index. - target_ssize_t elemOffset = GetFirstElemOffset(); - unsigned elemSizeUn = (GetElemType() == TYP_STRUCT) ? comp->typGetObjLayout(GetElemClassHandle())->GetSize() - : genTypeSize(GetElemType()); - - assert(FitsIn(elemSizeUn)); - target_ssize_t elemSize = static_cast(elemSizeUn); - target_ssize_t constIndexOffset = offset - elemOffset; + assert(FitsIn(GetElemSize())); + target_ssize_t elemSize = static_cast(GetElemSize()); + target_ssize_t constIndexOffset = offset - (GetFirstElemOffset() + GetFieldOffset()); // This should be divisible by the element size... assert((constIndexOffset % elemSize) == 0); diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 81c0c181bfa3ed..86271ae2f69750 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -192,6 +192,9 @@ struct GuardedDevirtualizationCandidateInfo; struct HandleHistogramProfileCandidateInfo; struct LateDevirtualizationInfo; +template +unsigned genTypeSize(T value); + typedef unsigned short AssertionIndex; static const AssertionIndex NO_ASSERTION_INDEX = 0; @@ -6425,17 +6428,27 @@ struct GenTreeArrAddr : GenTreeUnOp CORINFO_CLASS_HANDLE m_elemClassHandle; // The array element class. Currently only used for arrays of TYP_STRUCT. var_types m_elemType; // The normalized (TYP_SIMD != TYP_STRUCT) array element type. uint8_t m_firstElemOffset; // Offset to the first element of the array. + unsigned m_elemSize; // Size of the array element type. + unsigned m_fieldOffset; // Compound offset of the element's fields (if any). public: - GenTreeArrAddr(GenTree* addr, var_types elemType, CORINFO_CLASS_HANDLE elemClassHandle, uint8_t firstElemOffset) + GenTreeArrAddr(GenTree* addr, + var_types elemType, + CORINFO_CLASS_HANDLE elemClassHandle, + unsigned elemSize, + uint8_t firstElemOffset, + unsigned fieldOffset) : GenTreeUnOp(GT_ARR_ADDR, TYP_BYREF, addr DEBUGARG(/* largeNode */ false)) , m_elemClassHandle(elemClassHandle) , m_elemType(elemType) , m_firstElemOffset(firstElemOffset) + , m_elemSize(elemSize) + , m_fieldOffset(fieldOffset) { assert(addr->TypeIs(TYP_BYREF)); assert(((elemType == TYP_STRUCT) && (elemClassHandle != NO_CLASS_HANDLE)) || (elemClassHandle == NO_CLASS_HANDLE)); + assert((elemSize == genTypeSize(elemType)) || ((elemSize != 0) && (elemType == TYP_STRUCT))); } #if DEBUGGABLE_GENTREE @@ -6459,11 +6472,27 @@ struct GenTreeArrAddr : GenTreeUnOp return m_elemType; } + unsigned GetElemSize() const + { + return m_elemSize; + } + uint8_t GetFirstElemOffset() const { return m_firstElemOffset; } + unsigned GetFieldOffset() const + { + return m_fieldOffset; + } + + void AddField(unsigned offset) + { + assert((m_fieldOffset + offset) < m_elemSize); + m_fieldOffset += offset; + } + void ParseArrayAddress(Compiler* comp, GenTree** pArr, ValueNum* pInxVN); private: diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 101b7d0c421893..e649b8b54a9ca7 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -4686,7 +4686,7 @@ GenTree* Compiler::fgMorphIndexAddr(GenTreeIndexAddr* indexAddr) } // TODO-Throughput: bash the INDEX_ADDR to ARR_ADDR here instead of creating a new node. - addr = new (this, GT_ARR_ADDR) GenTreeArrAddr(addr, elemTyp, elemStructType, elemOffs); + addr = new (this, GT_ARR_ADDR) GenTreeArrAddr(addr, elemTyp, elemStructType, elemSize, elemOffs, 0); if (indexAddr->IsNotNull()) { @@ -12561,6 +12561,28 @@ GenTree* Compiler::fgOptimizeAddition(GenTreeOp* add) } } + // Sink offset addition below "ARR_ADDR"s to find potentially optimizable commutative folding. + // + if (fgGlobalMorph && op2->IsCnsIntOrI()) + { + GenTree* effectiveOp1 = op1->gtEffectiveVal(); + if (effectiveOp1->OperIs(GT_ARR_ADDR)) + { + GenTreeArrAddr* addr = effectiveOp1->AsArrAddr(); + ssize_t offset = op2->AsIntCon()->IconValue(); + if ((0 <= offset) && ((addr->GetFieldOffset() + offset) < addr->GetElemSize())) + { + add->SetAllEffectsFlags(addr); + add->gtOp1 = addr->gtOp1; + GenTree* optimizedAdd = fgOptimizeCommutativeArithmetic(add); + addr->gtOp1 = optimizedAdd; + addr->AddField(static_cast(offset)); + + return op1; + } + } + } + // - a + b = > b - a // ADD((NEG(a), b) => SUB(b, a) From cb4065eb779eb240d52c9bc3fa00e69eb70273bd Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 28 Aug 2022 01:00:21 +0300 Subject: [PATCH 2/4] Delete "GenTree::IsArrayAddr" No longer necessary. --- src/coreclr/jit/gentree.cpp | 30 ------------------------------ src/coreclr/jit/gentree.h | 2 -- src/coreclr/jit/optimizer.cpp | 13 +++++-------- 3 files changed, 5 insertions(+), 40 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 9b322681e1a5ad..f8166fa2587435 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -18325,36 +18325,6 @@ void GenTreeArrAddr::ParseArrayAddress(Compiler* comp, GenTree** pArr, ValueNum* } } -//------------------------------------------------------------------------ -// IsArrayAddr: Is "this" an expression for an array address? -// -// Recognizes the following patterns: -// this: ARR_ADDR -// this: ADD(ARR_ADDR, CONST) -// -// Arguments: -// pArrAddr - [out] parameter for the found ARR_ADDR node -// -// Return Value: -// Whether "this" matches the pattern denoted above. -// -bool GenTree::IsArrayAddr(GenTreeArrAddr** pArrAddr) -{ - GenTree* addr = this; - if (addr->OperIs(GT_ADD) && addr->AsOp()->gtGetOp2()->IsCnsIntOrI()) - { - addr = addr->AsOp()->gtGetOp1(); - } - - if (addr->OperIs(GT_ARR_ADDR)) - { - *pArrAddr = addr->AsArrAddr(); - return true; - } - - return false; -} - //------------------------------------------------------------------------ // Create: Create or retrieve a field sequence for the given field handle. // diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 86271ae2f69750..2349db7f1da9a7 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -2013,8 +2013,6 @@ struct GenTree bool IsFieldAddr(Compiler* comp, GenTree** pBaseAddr, FieldSeq** pFldSeq, ssize_t* pOffset); - bool IsArrayAddr(GenTreeArrAddr** pArrAddr); - // These are only used for dumping. // The GetRegNum() is only valid in LIR, but the dumping methods are not easily // modified to check this. diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 4deb904b989898..a726e315bf7ee3 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -8473,17 +8473,14 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) } else { - GenTreeArrAddr* arrAddr = nullptr; - GenTree* baseAddr = nullptr; - FieldSeq* fldSeq = nullptr; - ssize_t offset = 0; + GenTree* baseAddr = nullptr; + FieldSeq* fldSeq = nullptr; + ssize_t offset = 0; - if (arg->IsArrayAddr(&arrAddr)) + if (arg->OperIs(GT_ARR_ADDR)) { - // We will not collect "fldSeq" -- any modification to an S[], at - // any field of "S", will lose all information about the array type. CORINFO_CLASS_HANDLE elemTypeEq = - EncodeElemType(arrAddr->GetElemType(), arrAddr->GetElemClassHandle()); + EncodeElemType(arg->AsArrAddr()->GetElemType(), arg->AsArrAddr()->GetElemClassHandle()); AddModifiedElemTypeAllContainingLoops(mostNestedLoop, elemTypeEq); // Conservatively assume byrefs may alias this array element memoryHavoc |= memoryKindSet(ByrefExposed); From 5d22e1d8077245af84865cd688968b45b9f61212 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 28 Aug 2022 20:48:31 +0300 Subject: [PATCH 3/4] Delete a bit of obsolete code while we're here --- src/coreclr/jit/gentree.cpp | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index f8166fa2587435..e4e99f33a17303 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -18243,25 +18243,11 @@ void GenTreeArrAddr::ParseArrayAddress(Compiler* comp, GenTree** pArr, ValueNum* GenTree* nonConst = nullptr; if (tree->AsOp()->gtOp1->IsCnsIntOrI()) { - // If the other arg is an int constant, and is a "not-a-field", choose - // that as the multiplier, thus preserving constant index offsets... - if (tree->AsOp()->gtOp2->OperGet() == GT_CNS_INT && - tree->AsOp()->gtOp2->AsIntCon()->gtFieldSeq == nullptr) - { - assert(!tree->AsOp()->gtOp2->AsIntCon()->ImmedValNeedsReloc(comp)); - // TODO-CrossBitness: we wouldn't need the cast below if GenTreeIntConCommon::gtIconVal had - // target_ssize_t type. - subMul = (target_ssize_t)tree->AsOp()->gtOp2->AsIntConCommon()->IconValue(); - nonConst = tree->AsOp()->gtOp1; - } - else - { - assert(!tree->AsOp()->gtOp1->AsIntCon()->ImmedValNeedsReloc(comp)); - // TODO-CrossBitness: we wouldn't need the cast below if GenTreeIntConCommon::gtIconVal had - // target_ssize_t type. - subMul = (target_ssize_t)tree->AsOp()->gtOp1->AsIntConCommon()->IconValue(); - nonConst = tree->AsOp()->gtOp2; - } + assert(!tree->AsOp()->gtOp1->AsIntCon()->ImmedValNeedsReloc(comp)); + // TODO-CrossBitness: we wouldn't need the cast below if GenTreeIntConCommon::gtIconVal had + // target_ssize_t type. + subMul = (target_ssize_t)tree->AsOp()->gtOp1->AsIntConCommon()->IconValue(); + nonConst = tree->AsOp()->gtOp2; } else if (tree->AsOp()->gtOp2->IsCnsIntOrI()) { From bcf77c1fcafe0629a16b65f2eb74e883d07b589c Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 28 Aug 2022 22:10:38 +0300 Subject: [PATCH 4/4] Fix bugs --- src/coreclr/jit/gentree.h | 5 ++--- src/coreclr/jit/morph.cpp | 19 ++++++++----------- src/coreclr/jit/valuenum.cpp | 2 +- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 2349db7f1da9a7..d98032c5a74e5e 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -6485,10 +6485,9 @@ struct GenTreeArrAddr : GenTreeUnOp return m_fieldOffset; } - void AddField(unsigned offset) + void AddOffset(ssize_t offset) { - assert((m_fieldOffset + offset) < m_elemSize); - m_fieldOffset += offset; + m_fieldOffset = (m_fieldOffset + static_cast(offset)) % m_elemSize; } void ParseArrayAddress(Compiler* comp, GenTree** pArr, ValueNum* pInxVN); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index e649b8b54a9ca7..fe17b7784069f2 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -12492,7 +12492,7 @@ GenTree* Compiler::fgOptimizeAddition(GenTreeOp* add) // Fold "((x + icon1) + (y + icon2))" to ((x + y) + (icon1 + icon2))". // Be careful not to create a byref pointer that may point outside of the ref object. - // Only do this in global morph as we don't recompute the VN for "(x + y)", the new "op2". + // Only do this in global morph as we don't recompute the VN for "(x + y)", the new "op1". if (op1->OperIs(GT_ADD) && op2->OperIs(GT_ADD) && !op1->gtOverflow() && !op2->gtOverflow() && op1->AsOp()->gtGetOp2()->IsCnsIntOrI() && op2->AsOp()->gtGetOp2()->IsCnsIntOrI() && !varTypeIsGC(op1->AsOp()->gtGetOp1()) && !varTypeIsGC(op2->AsOp()->gtGetOp1()) && fgGlobalMorph) @@ -12562,7 +12562,6 @@ GenTree* Compiler::fgOptimizeAddition(GenTreeOp* add) } // Sink offset addition below "ARR_ADDR"s to find potentially optimizable commutative folding. - // if (fgGlobalMorph && op2->IsCnsIntOrI()) { GenTree* effectiveOp1 = op1->gtEffectiveVal(); @@ -12570,16 +12569,14 @@ GenTree* Compiler::fgOptimizeAddition(GenTreeOp* add) { GenTreeArrAddr* addr = effectiveOp1->AsArrAddr(); ssize_t offset = op2->AsIntCon()->IconValue(); - if ((0 <= offset) && ((addr->GetFieldOffset() + offset) < addr->GetElemSize())) - { - add->SetAllEffectsFlags(addr); - add->gtOp1 = addr->gtOp1; - GenTree* optimizedAdd = fgOptimizeCommutativeArithmetic(add); - addr->gtOp1 = optimizedAdd; - addr->AddField(static_cast(offset)); - return op1; - } + add->SetAllEffectsFlags(addr); + add->gtOp1 = addr->gtOp1; + GenTree* optimizedAdd = fgOptimizeCommutativeArithmetic(add); + addr->gtOp1 = optimizedAdd; + addr->AddOffset(offset); + + return op1; } } diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index c607f54052be7e..a1a5be7740f750 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -9173,7 +9173,7 @@ void Compiler::fgValueNumberArrIndexAddr(GenTreeArrAddr* arrAddr) ValueNum arrVN = vnStore->VNNormalValue(arr->GetVN(VNK_Liberal)); inxVN = vnStore->VNNormalValue(inxVN); - ValueNum offsetVN = vnStore->VNForIntPtrCon(0); + ValueNum offsetVN = vnStore->VNForIntPtrCon(arrAddr->GetFieldOffset()); ValueNum arrAddrVN = vnStore->VNForFunc(TYP_BYREF, VNF_PtrToArrElem, elemTypeEqVN, arrVN, inxVN, offsetVN); ValueNumPair arrAddrVNP = ValueNumPair(arrAddrVN, arrAddrVN);