From 8a99adad39fdf64df08993d9d5eefb1dc2168247 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 5 Jul 2024 13:41:37 +0200 Subject: [PATCH] JIT: Disable old promotion for structs with significant padding (#104438) When old promotion kicks in for structs with significant padding it generally results in dependent promotion as block morphing handles these conservatively. Disable this case and allow physical promotion to handle it instead. --- src/coreclr/jit/compiler.h | 8 -------- src/coreclr/jit/lclvars.cpp | 33 ++++++++++++++------------------- src/coreclr/jit/liveness.cpp | 2 +- src/coreclr/jit/morph.cpp | 11 +++++------ src/coreclr/jit/morphblock.cpp | 22 ---------------------- 5 files changed, 20 insertions(+), 56 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index fb5950694f5..f2ea5ec78ca 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -605,12 +605,6 @@ public: unsigned char lvIsStructField : 1; // Is this local var a field of a promoted struct local? unsigned char lvContainsHoles : 1; // Is this a promoted struct whose fields do not cover the struct local? - // True for a promoted struct that has significant padding in it. - // Significant padding is any data in the struct that is not covered by a - // promoted field and that the EE told us we need to preserve on block - // copies/inits. - unsigned char lvAnySignificantPadding : 1; - unsigned char lvIsMultiRegArg : 1; // true if this is a multireg LclVar struct used in an argument context unsigned char lvIsMultiRegRet : 1; // true if this is a multireg LclVar struct assigned from a multireg call @@ -4210,7 +4204,6 @@ public: CORINFO_CLASS_HANDLE typeHnd; bool canPromote; bool containsHoles; - bool anySignificantPadding; bool fieldsSorted; unsigned char fieldCnt; lvaStructFieldInfo fields[MAX_NumOfFieldsInPromotableStruct]; @@ -4219,7 +4212,6 @@ public: : typeHnd(typeHnd) , canPromote(false) , containsHoles(false) - , anySignificantPadding(false) , fieldsSorted(false) , fieldCnt(0) { diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 52623e920b5..87f67e1e330 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -2371,9 +2371,15 @@ bool Compiler::StructPromotionHelper::CanPromoteStructType(CORINFO_CLASS_HANDLE if (fieldsSize != treeNodes[0].size) { structPromotionInfo.containsHoles = true; - } - structPromotionInfo.anySignificantPadding = treeNodes[0].hasSignificantPadding && structPromotionInfo.containsHoles; + if (treeNodes[0].hasSignificantPadding) + { + // Struct has significant data not covered by fields we would promote; + // this would typically result in dependent promotion, so leave this + // struct to physical promotion. + return false; + } + } // Cool, this struct is promotable. @@ -2716,11 +2722,6 @@ bool Compiler::StructPromotionHelper::ShouldPromoteStructVar(unsigned lclNum) structPromotionInfo.fieldCnt, varDsc->lvFieldAccessed); shouldPromote = false; } - else if (varDsc->lvIsMultiRegRet && structPromotionInfo.anySignificantPadding) - { - JITDUMP("Not promoting multi-reg returned struct local V%02u with significant padding.\n", lclNum); - shouldPromote = false; - } #if defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) else if ((structPromotionInfo.fieldCnt == 2) && (varTypeIsFloating(structPromotionInfo.fields[0].fldType) || varTypeIsFloating(structPromotionInfo.fields[1].fldType))) @@ -2739,13 +2740,8 @@ bool Compiler::StructPromotionHelper::ShouldPromoteStructVar(unsigned lclNum) // multiple registers? if (compiler->lvaIsMultiregStruct(varDsc, compiler->info.compIsVarArgs)) { - if (structPromotionInfo.anySignificantPadding) - { - JITDUMP("Not promoting multi-reg struct local V%02u with significant padding.\n", lclNum); - shouldPromote = false; - } - else if ((structPromotionInfo.fieldCnt != 2) && - !((structPromotionInfo.fieldCnt == 1) && varTypeIsSIMD(structPromotionInfo.fields[0].fldType))) + if ((structPromotionInfo.fieldCnt != 2) && + !((structPromotionInfo.fieldCnt == 1) && varTypeIsSIMD(structPromotionInfo.fields[0].fldType))) { JITDUMP("Not promoting multireg struct local V%02u, because lvIsParam is true, #fields != 2 and it's " "not a single SIMD.\n", @@ -2834,11 +2830,10 @@ void Compiler::StructPromotionHelper::PromoteStructVar(unsigned lclNum) assert(varDsc->GetLayout()->GetClassHandle() == structPromotionInfo.typeHnd); assert(structPromotionInfo.canPromote); - varDsc->lvFieldCnt = structPromotionInfo.fieldCnt; - varDsc->lvFieldLclStart = compiler->lvaCount; - varDsc->lvPromoted = true; - varDsc->lvContainsHoles = structPromotionInfo.containsHoles; - varDsc->lvAnySignificantPadding = structPromotionInfo.anySignificantPadding; + varDsc->lvFieldCnt = structPromotionInfo.fieldCnt; + varDsc->lvFieldLclStart = compiler->lvaCount; + varDsc->lvPromoted = true; + varDsc->lvContainsHoles = structPromotionInfo.containsHoles; #ifdef DEBUG // Don't stress this in LCL_FLD stress. diff --git a/src/coreclr/jit/liveness.cpp b/src/coreclr/jit/liveness.cpp index a9f566bd37b..575b85813ee 100644 --- a/src/coreclr/jit/liveness.cpp +++ b/src/coreclr/jit/liveness.cpp @@ -1024,7 +1024,7 @@ bool Compiler::fgComputeLifeUntrackedLocal(VARSET_TP& life, { // Do not consider this store dead if the parent local variable is an address exposed local or // if the struct has any significant padding we must retain the value of. - return !varDsc.IsAddressExposed() && !varDsc.lvAnySignificantPadding; + return !varDsc.IsAddressExposed(); } return false; diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 1805435c5ad..6798832d1f8 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14728,12 +14728,11 @@ PhaseStatus Compiler::fgRetypeImplicitByRefArgs() } // Copy the struct promotion annotations to the new temp. - LclVarDsc* newVarDsc = lvaGetDesc(newLclNum); - newVarDsc->lvPromoted = true; - newVarDsc->lvFieldLclStart = varDsc->lvFieldLclStart; - newVarDsc->lvFieldCnt = varDsc->lvFieldCnt; - newVarDsc->lvContainsHoles = varDsc->lvContainsHoles; - newVarDsc->lvAnySignificantPadding = varDsc->lvAnySignificantPadding; + LclVarDsc* newVarDsc = lvaGetDesc(newLclNum); + newVarDsc->lvPromoted = true; + newVarDsc->lvFieldLclStart = varDsc->lvFieldLclStart; + newVarDsc->lvFieldCnt = varDsc->lvFieldCnt; + newVarDsc->lvContainsHoles = varDsc->lvContainsHoles; #ifdef DEBUG newVarDsc->lvKeepType = true; #endif // DEBUG diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index 0f49e62c647..f21cc2355a4 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -359,12 +359,6 @@ void MorphInitBlockHelper::TryInitFieldByField() return; } - if (destLclVar->lvAnySignificantPadding) - { - JITDUMP(" dest has significant padding.\n"); - return; - } - if (m_dstLclOffset != 0) { JITDUMP(" dest not at a zero offset.\n"); @@ -775,22 +769,6 @@ void MorphCopyBlockHelper::MorphStructCases() requiresCopyBlock = true; } - // Can we use field by field copy for the dest? - if (m_dstDoFldStore && m_dstVarDsc->lvAnySignificantPadding) - { - JITDUMP(" dest has significant padding"); - // C++ style CopyBlock with holes - requiresCopyBlock = true; - } - - // Can we use field by field copy for the src? - if (m_srcDoFldStore && m_srcVarDsc->lvAnySignificantPadding) - { - JITDUMP(" src has significant padding"); - // C++ style CopyBlock with holes - requiresCopyBlock = true; - } - #if defined(TARGET_ARM) if (m_store->OperIsIndir() && m_store->AsIndir()->IsUnaligned()) {