From 1498da4428b5bb7ab92cf1940dc0b9521aafcf68 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 20 Oct 2022 01:45:28 +0300 Subject: [PATCH 1/8] Remove quirks --- src/coreclr/jit/copyprop.cpp | 16 ++++------------ src/coreclr/jit/valuenum.cpp | 5 ----- 2 files changed, 4 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index d029c8f3dcef01..dd86e944af54c3 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -315,20 +315,12 @@ void Compiler::optCopyPropPushDef(GenTree* defNode, GenTreeLclVarCommon* lclNode LclVarDsc* varDsc = lvaGetDesc(lclNum); assert(varDsc->lvPromoted); - if (varDsc->CanBeReplacedWithItsField(this)) + for (unsigned index = 0; index < varDsc->lvFieldCnt; index++) { - // TODO-CQ: remove this zero-diff quirk. - pushDef(varDsc->lvFieldLclStart, SsaConfig::RESERVED_SSA_NUM); - } - else - { - for (unsigned index = 0; index < varDsc->lvFieldCnt; index++) + unsigned ssaNum = lclNode->GetSsaNum(this, index); + if (ssaNum != SsaConfig::RESERVED_SSA_NUM) { - unsigned ssaNum = lclNode->GetSsaNum(this, index); - if (ssaNum != SsaConfig::RESERVED_SSA_NUM) - { - pushDef(varDsc->lvFieldLclStart + index, ssaNum); - } + pushDef(varDsc->lvFieldLclStart + index, ssaNum); } } } diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 791305c22d3387..075a0d962cac8d 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -8439,11 +8439,6 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree) rhsVNPair.SetBoth(initObjVN); } - else if (lhs->OperIs(GT_LCL_VAR) && lhsVarDsc->CanBeReplacedWithItsField(this)) - { - // TODO-CQ: remove this zero-diff quirk. - rhsVNPair.SetBoth(vnStore->VNForExpr(compCurBB, lvaGetDesc(lhsVarDsc->lvFieldLclStart)->TypeGet())); - } else { assert(tree->OperIsCopyBlkOp()); From dba93b8128c35ced8f958976aed6775ebf91c05f Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 25 Sep 2022 00:03:51 +0300 Subject: [PATCH 2/8] Early prop fix for call asgs --- src/coreclr/jit/earlyprop.cpp | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/earlyprop.cpp b/src/coreclr/jit/earlyprop.cpp index 5c30adcdb7cde6..6d723fdececf82 100644 --- a/src/coreclr/jit/earlyprop.cpp +++ b/src/coreclr/jit/earlyprop.cpp @@ -361,13 +361,10 @@ GenTree* Compiler::optPropGetValueRec(unsigned lclNum, unsigned ssaNum, optPropK LclSsaVarDsc* ssaVarDsc = lvaTable[lclNum].GetPerSsaData(ssaNum); GenTreeOp* ssaDefAsg = ssaVarDsc->GetAssignment(); - if (ssaDefAsg == nullptr) - { - // Incoming parameters or live-in variables don't have actual definition tree node - // for their FIRST_SSA_NUM. See SsaBuilder::RenameVariables. - assert(ssaNum == SsaConfig::FIRST_SSA_NUM); - } - else + // Incoming parameters or live-in variables don't have actual definition tree node for + // their FIRST_SSA_NUM. Definitions induced by calls do not record the store node. See + // SsaBuilder::RenameDef. + if (ssaDefAsg != nullptr) { assert(ssaDefAsg->OperIs(GT_ASG)); @@ -565,8 +562,19 @@ GenTree* Compiler::optFindNullCheckToFold(GenTree* tree, LocalNumberToNullCheckT return nullptr; } - GenTree* defRHS = defLoc->GetAssignment()->gtGetOp2(); + GenTree* defNode = defLoc->GetAssignment(); + if (defNode == nullptr) + { + return nullptr; + } + + GenTree* defLHS = defNode->gtGetOp1(); + if (!defLHS->OperIs(GT_LCL_VAR) || (defLHS->AsLclVar()->GetLclNum() != lclNum)) + { + return nullptr; + } + GenTree* defRHS = defNode->gtGetOp2(); if (defRHS->OperGet() != GT_COMMA) { return nullptr; From 0273062974e4e280f58cca33e1eeb675aaf45361 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 25 Sep 2022 00:04:12 +0300 Subject: [PATCH 3/8] Include all tracked locals in SSA --- src/coreclr/jit/ssabuilder.cpp | 41 +--------------------------------- src/coreclr/jit/ssabuilder.h | 2 -- 2 files changed, 1 insertion(+), 42 deletions(-) diff --git a/src/coreclr/jit/ssabuilder.cpp b/src/coreclr/jit/ssabuilder.cpp index e57a3423ec6d1b..624f1a0212b44e 100644 --- a/src/coreclr/jit/ssabuilder.cpp +++ b/src/coreclr/jit/ssabuilder.cpp @@ -1597,7 +1597,7 @@ void SsaBuilder::Build() // Mark all variables that will be tracked by SSA for (unsigned lclNum = 0; lclNum < m_pCompiler->lvaCount; lclNum++) { - m_pCompiler->lvaTable[lclNum].lvInSsa = IncludeInSsa(lclNum); + m_pCompiler->lvaTable[lclNum].lvInSsa = m_pCompiler->lvaGetDesc(lclNum)->lvTracked; } // Insert phi functions. @@ -1660,45 +1660,6 @@ void SsaBuilder::SetupBBRoot() } } -//------------------------------------------------------------------------ -// IncludeInSsa: Check if the specified variable can be included in SSA. -// -// Arguments: -// lclNum - the variable number -// -// Return Value: -// true if the variable is included in SSA -// -bool SsaBuilder::IncludeInSsa(unsigned lclNum) -{ - LclVarDsc* varDsc = m_pCompiler->lvaGetDesc(lclNum); - - if (!varDsc->lvTracked) - { - return false; // SSA is only done for tracked variables - } - // lvPromoted structs are never tracked... - assert(!varDsc->lvPromoted); - - if (varDsc->lvIsStructField && - (m_pCompiler->lvaGetParentPromotionType(lclNum) != Compiler::PROMOTION_TYPE_INDEPENDENT)) - { - // SSA must exclude struct fields that are not independent - // - because we don't model the struct assignment properly when multiple fields can be assigned by one struct - // assignment. - // - SSA doesn't allow a single node to contain multiple SSA definitions. - // - and PROMOTION_TYPE_DEPENDEDNT fields are never candidates for a register. - // - return false; - } - else if (varDsc->lvIsStructField && m_pCompiler->lvaGetDesc(varDsc->lvParentLcl)->lvIsMultiRegRet) - { - return false; - } - // otherwise this variable is included in SSA - return true; -} - #ifdef DEBUG // This method asserts that SSA name constraints specified are satisfied. void Compiler::JitTestCheckSSA() diff --git a/src/coreclr/jit/ssabuilder.h b/src/coreclr/jit/ssabuilder.h index c84da01f27526c..b89a1d41d95f0e 100644 --- a/src/coreclr/jit/ssabuilder.h +++ b/src/coreclr/jit/ssabuilder.h @@ -20,8 +20,6 @@ class SsaBuilder m_pCompiler->EndPhase(phase); } - bool IncludeInSsa(unsigned lclNum); - public: // Constructor SsaBuilder(Compiler* pCompiler); From f6da0adf8e6d49b0d1163d58b388313c0f6be651 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 20 Oct 2022 02:02:58 +0300 Subject: [PATCH 4/8] Remove a (now incorrect) comment --- src/coreclr/jit/morphblock.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index 288f55657cc4d0..307e48559bcc96 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -806,8 +806,6 @@ void MorphCopyBlockHelper::TrySpecialCases() { assert(m_dst->OperIs(GT_LCL_VAR)); - // This will exclude field locals (if any) from SSA: we do not have a way to - // associate multiple SSA definitions (SSA numbers) with one store. m_dstVarDsc->lvIsMultiRegRet = true; JITDUMP("Not morphing a multireg node return\n"); From af6327cf77718462d091446c4fe61199f6b9055f Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 20 Oct 2022 09:24:44 +0300 Subject: [PATCH 5/8] Fix a typo --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/gentree.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 06dda1670c68d8..e14a0905db7799 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -2771,7 +2771,7 @@ class Compiler bool gtIsStaticFieldPtrToBoxedStruct(var_types fieldNodeType, CORINFO_FIELD_HANDLE fldHnd); bool gtStoreDefinesField( - LclVarDsc* fieldVarDsc, ssize_t offset, unsigned size, ssize_t* pFieldStoreOffset, unsigned* pFileStoreSize); + LclVarDsc* fieldVarDsc, ssize_t offset, unsigned size, ssize_t* pFieldStoreOffset, unsigned* pFieldStoreSize); // Return true if call is a recursive call; return false otherwise. // Note when inlining, this looks for calls back to the root method. diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 2f511fa8807613..9afa95fe6543a2 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17399,7 +17399,7 @@ bool Compiler::gtIsStaticFieldPtrToBoxedStruct(var_types fieldNodeType, CORINFO_ // size - Size of the store in bytes // pFieldStoreOffset - [out] parameter for the store's offset relative // to the field local itself -// pFileStoreSize - [out] parameter for the amount of the field's +// pFieldStoreSize - [out] parameter for the amount of the field's // local's bytes affected by the store // // Return Value: @@ -17407,7 +17407,7 @@ bool Compiler::gtIsStaticFieldPtrToBoxedStruct(var_types fieldNodeType, CORINFO_ // otherwise. // bool Compiler::gtStoreDefinesField( - LclVarDsc* fieldVarDsc, ssize_t offset, unsigned size, ssize_t* pFieldStoreOffset, unsigned* pFileStoreSize) + LclVarDsc* fieldVarDsc, ssize_t offset, unsigned size, ssize_t* pFieldStoreOffset, unsigned* pFieldStoreSize) { ssize_t fieldOffset = fieldVarDsc->lvFldOffset; unsigned fieldSize = genTypeSize(fieldVarDsc); // No TYP_STRUCT field locals. @@ -17417,7 +17417,7 @@ bool Compiler::gtStoreDefinesField( if ((fieldOffset < storeEndOffset) && (offset < fieldEndOffset)) { *pFieldStoreOffset = (offset < fieldOffset) ? 0 : (offset - fieldOffset); - *pFileStoreSize = static_cast(min(storeEndOffset, fieldEndOffset) - max(offset, fieldOffset)); + *pFieldStoreSize = static_cast(min(storeEndOffset, fieldEndOffset) - max(offset, fieldOffset)); return true; } From 99ed47c9666ea7ca753b827c7a5763324a877c76 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 20 Oct 2022 19:53:30 +0300 Subject: [PATCH 6/8] Encoding fixes 1) The definition of SIMPLE_NUM_COUNT was wrong. 2) SsaNumInfo::Composite, in the compact case, did not clear the old value. 3) SsaNumInfo::Composite, in the outlined case, did not copy the already (compactly) encoded simple names. --- src/coreclr/jit/gentree.cpp | 26 +++++++++++++++++++------- src/coreclr/jit/gentree.h | 2 +- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 9afa95fe6543a2..ca3f3cf454da6a 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -23490,10 +23490,10 @@ unsigned* SsaNumInfo::GetOutlinedNumSlot(Compiler* compiler, unsigned index) con return SsaNumInfo(COMPOSITE_ENCODING_BIT | ssaNumEncoded); } - return SsaNumInfo(ssaNumEncoded | baseNum.m_value); + return SsaNumInfo(ssaNumEncoded | (baseNum.m_value & ~(SIMPLE_NUM_MASK << (index * BITS_PER_SIMPLE_NUM)))); } - if (!baseNum.IsInvalid()) + if (!baseNum.IsInvalid() && !baseNum.HasCompactFormat()) { *baseNum.GetOutlinedNumSlot(compiler, index) = ssaNum; return baseNum; @@ -23507,11 +23507,23 @@ unsigned* SsaNumInfo::GetOutlinedNumSlot(Compiler* compiler, unsigned index) con } // Allocate a new chunk for the field numbers. Once allocated, it cannot be expanded. - int count = compiler->lvaGetDesc(parentLclNum)->lvFieldCnt; - JitExpandArrayStack* table = compiler->m_outlinedCompositeSsaNums; - int outIdx = table->Size(); - unsigned* pLastSlot = &table->GetRef(outIdx + count - 1); // This will grow the table. - pLastSlot[-(count - 1) + static_cast(index)] = ssaNum; + int count = compiler->lvaGetDesc(parentLclNum)->lvFieldCnt; + JitExpandArrayStack* table = compiler->m_outlinedCompositeSsaNums; + int outIdx = table->Size(); + unsigned* pLastSlot = &table->GetRef(outIdx + count - 1); // This will grow the table. + unsigned* pFirstSlot = pLastSlot - count + 1; + + // Copy over all of the already encoded numbers. + if (!baseNum.IsInvalid()) + { + for (int i = 0; i < SIMPLE_NUM_COUNT; i++) + { + pFirstSlot[i] = baseNum.GetNum(compiler, i); + } + } + + // Copy the one being set last to overwrite any previous values. + pFirstSlot[index] = ssaNum; // Split the index if it does not fit into a small encoding. if ((outIdx & ~OUTLINED_INDEX_LOW_MASK) != 0) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 240a9bf7329616..d4af116b12a4fc 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -3538,7 +3538,7 @@ class SsaNumInfo final static const int BITS_PER_SIMPLE_NUM = 8; static const int MAX_SIMPLE_NUM = (1 << (BITS_PER_SIMPLE_NUM - 1)) - 1; static const int SIMPLE_NUM_MASK = MAX_SIMPLE_NUM; - static const int SIMPLE_NUM_COUNT = sizeof(int) / BITS_PER_SIMPLE_NUM; + static const int SIMPLE_NUM_COUNT = (sizeof(int) * BITS_PER_BYTE) / BITS_PER_SIMPLE_NUM; static const int COMPOSITE_ENCODING_BIT = 1 << 31; static const int OUTLINED_ENCODING_BIT = 1 << 15; static const int OUTLINED_INDEX_LOW_MASK = OUTLINED_ENCODING_BIT - 1; From 44238c856b3296081f76e4a6318a658faf598460 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 20 Oct 2022 20:53:05 +0300 Subject: [PATCH 7/8] Fix store numbering The load path needs to use the offset relative to the store's target location. --- src/coreclr/jit/valuenum.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 075a0d962cac8d..c90c7657f234dc 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4917,8 +4917,10 @@ void Compiler::fgValueNumberLocalStore(GenTree* storeNode, // Avoid redundant bitcasts for the common case of a full definition. fieldStoreType = fieldVarDsc->TypeGet(); } + + ssize_t fieldValueOffset = max(0, fieldVarDsc->lvFldOffset - offset); ValueNumPair fieldStoreValue = - vnStore->VNPairForLoad(value, storeSize, fieldStoreType, offset, fieldStoreSize); + vnStore->VNPairForLoad(value, storeSize, fieldStoreType, fieldValueOffset, fieldStoreSize); processDef(fieldLclNum, lclDefNode->GetSsaNum(this, index), fieldStoreOffset, fieldStoreSize, fieldStoreValue); From dd886206f8540bb416afa59e5ac8cb329713c8c8 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Fri, 4 Nov 2022 22:05:34 +0300 Subject: [PATCH 8/8] Clarify 'fieldValueOffset' calculations --- src/coreclr/jit/valuenum.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index c90c7657f234dc..0f9df4bab9a7de 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4918,7 +4918,9 @@ void Compiler::fgValueNumberLocalStore(GenTree* storeNode, fieldStoreType = fieldVarDsc->TypeGet(); } - ssize_t fieldValueOffset = max(0, fieldVarDsc->lvFldOffset - offset); + // Calculate offset of this field's value, relative to the entire one. + ssize_t fieldOffset = fieldVarDsc->lvFldOffset; + ssize_t fieldValueOffset = (fieldOffset < offset) ? 0 : (fieldOffset - offset); ValueNumPair fieldStoreValue = vnStore->VNPairForLoad(value, storeSize, fieldStoreType, fieldValueOffset, fieldStoreSize);