From dc0e333808ad78084d9b881dd6c69fe557f28f58 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 27 Jan 2022 14:53:10 +0300 Subject: [PATCH 1/3] Relax the type check The type check is too conservative: it prevents partial definitions from being used in propagation: ``` LCL_FLD V00/1 [X] = { ... }; // Pushed on the stack as a def. USE LCL_VAR V01 // Has the same VN as V00/1, but the type // check prevented it from being replaced. ``` This new version is conservative too, but will do for now as we don't propagate on (most) partial uses. Another reason for this change is that in my upcoming refactoring of copy propagation (that will bring another 0.5% in TP gains), we will no longer have the "defNode" available. --- src/coreclr/jit/copyprop.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index 52c765a12dc41e..9b3e0cb386c798 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -172,11 +172,6 @@ void Compiler::optCopyProp(Statement* stmt, continue; } - if (newLclDefNode->TypeGet() != tree->TypeGet()) - { - continue; - } - if (newLclDefVN != tree->gtVNPair.GetConservative()) { continue; @@ -225,6 +220,17 @@ void Compiler::optCopyProp(Statement* stmt, continue; } + var_types newLclType = newLclVarDsc->TypeGet(); + if (!newLclVarDsc->lvNormalizeOnLoad()) + { + newLclType = genActualType(newLclType); + } + + if (newLclType != tree->TypeGet()) + { + continue; + } + #ifdef DEBUG if (verbose) { From f7be01cfa7529929361e0f9e9f6b4208f51ae198 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 27 Jan 2022 15:32:55 +0300 Subject: [PATCH 2/3] Do not propagate shadowed "this" Ordinarily, shadowed parameters would not be used for propagation anyway, because of the liveness check, but "this" bypasses that checks, and so was used, which is presumably not what we want. Regardless of that, it is also not profitable to propagate "this" in such a situation as it extends its live range and makes the RA unhappy. --- src/coreclr/jit/compiler.h | 2 ++ src/coreclr/jit/copyprop.cpp | 2 +- src/coreclr/jit/gschecks.cpp | 10 ++++------ 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 69d88c4a57e280..ec0eac340aaac4 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -10843,6 +10843,8 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX public: struct ShadowParamVarInfo { + static const unsigned NO_SHADOW_COPY = UINT_MAX; + FixedBitVect* assignGroup; // the closure set of variables whose values depend on each other unsigned shadowCopy; // Lcl var num, valid only if not set to NO_SHADOW_COPY diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index 9b3e0cb386c798..004d82e6507b13 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -161,7 +161,7 @@ void Compiler::optCopyProp(Statement* stmt, } if ((gsShadowVarInfo != nullptr) && newLclVarDsc->lvIsParam && - (gsShadowVarInfo[newLclNum].shadowCopy == lclNum)) + (gsShadowVarInfo[newLclNum].shadowCopy != ShadowParamVarInfo::NO_SHADOW_COPY)) { continue; } diff --git a/src/coreclr/jit/gschecks.cpp b/src/coreclr/jit/gschecks.cpp index 7765231a87551d..2f4c40d1abd7b3 100644 --- a/src/coreclr/jit/gschecks.cpp +++ b/src/coreclr/jit/gschecks.cpp @@ -32,8 +32,6 @@ void Compiler::gsGSChecksInitCookie() info.compCompHnd->getGSCookie(&gsGlobalSecurityCookieVal, &gsGlobalSecurityCookieAddr); } -const unsigned NO_SHADOW_COPY = UINT_MAX; - /***************************************************************************** * gsCopyShadowParams * The current function has an unsafe buffer on the stack. Search for vulnerable @@ -368,7 +366,7 @@ void Compiler::gsParamsToShadows() for (UINT lclNum = 0; lclNum < lvaOldCount; lclNum++) { LclVarDsc* varDsc = lvaGetDesc(lclNum); - gsShadowVarInfo[lclNum].shadowCopy = NO_SHADOW_COPY; + gsShadowVarInfo[lclNum].shadowCopy = ShadowParamVarInfo::NO_SHADOW_COPY; // Only care about params whose values are on the stack if (!ShadowParamVarInfo::mayNeedShadowCopy(varDsc)) @@ -452,7 +450,7 @@ void Compiler::gsParamsToShadows() unsigned int lclNum = tree->AsLclVarCommon()->GetLclNum(); unsigned int shadowLclNum = m_compiler->gsShadowVarInfo[lclNum].shadowCopy; - if (shadowLclNum != NO_SHADOW_COPY) + if (shadowLclNum != Compiler::ShadowParamVarInfo::NO_SHADOW_COPY) { LclVarDsc* varDsc = m_compiler->lvaGetDesc(lclNum); assert(ShadowParamVarInfo::mayNeedShadowCopy(varDsc)); @@ -492,7 +490,7 @@ void Compiler::gsParamsToShadows() const LclVarDsc* varDsc = lvaGetDesc(lclNum); const unsigned shadowVarNum = gsShadowVarInfo[lclNum].shadowCopy; - if (shadowVarNum == NO_SHADOW_COPY) + if (shadowVarNum == ShadowParamVarInfo::NO_SHADOW_COPY) { continue; } @@ -544,7 +542,7 @@ void Compiler::gsParamsToShadows() const LclVarDsc* varDsc = lvaGetDesc(lclNum); const unsigned shadowVarNum = gsShadowVarInfo[lclNum].shadowCopy; - if (shadowVarNum == NO_SHADOW_COPY) + if (shadowVarNum == ShadowParamVarInfo::NO_SHADOW_COPY) { continue; } From dc5ac35ac4adafd7cfae7cb8b6fc48222c79832d Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Wed, 9 Feb 2022 12:38:31 +0300 Subject: [PATCH 3/3] NO_SHADOW_COPY -> BAD_VAR_NUM --- src/coreclr/jit/compiler.h | 4 +--- src/coreclr/jit/copyprop.cpp | 2 +- src/coreclr/jit/gschecks.cpp | 8 ++++---- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 0d82ab99e73310..ec344899439fa1 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -10858,10 +10858,8 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX public: struct ShadowParamVarInfo { - static const unsigned NO_SHADOW_COPY = UINT_MAX; - FixedBitVect* assignGroup; // the closure set of variables whose values depend on each other - unsigned shadowCopy; // Lcl var num, valid only if not set to NO_SHADOW_COPY + unsigned shadowCopy; // Lcl var num, if not valid set to BAD_VAR_NUM static bool mayNeedShadowCopy(LclVarDsc* varDsc) { diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index 3a7c0156ff939b..ab29e9889a7287 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -161,7 +161,7 @@ void Compiler::optCopyProp(Statement* stmt, } if ((gsShadowVarInfo != nullptr) && newLclVarDsc->lvIsParam && - (gsShadowVarInfo[newLclNum].shadowCopy != ShadowParamVarInfo::NO_SHADOW_COPY)) + (gsShadowVarInfo[newLclNum].shadowCopy != BAD_VAR_NUM)) { continue; } diff --git a/src/coreclr/jit/gschecks.cpp b/src/coreclr/jit/gschecks.cpp index 2f4c40d1abd7b3..60634f60cfcea8 100644 --- a/src/coreclr/jit/gschecks.cpp +++ b/src/coreclr/jit/gschecks.cpp @@ -366,7 +366,7 @@ void Compiler::gsParamsToShadows() for (UINT lclNum = 0; lclNum < lvaOldCount; lclNum++) { LclVarDsc* varDsc = lvaGetDesc(lclNum); - gsShadowVarInfo[lclNum].shadowCopy = ShadowParamVarInfo::NO_SHADOW_COPY; + gsShadowVarInfo[lclNum].shadowCopy = BAD_VAR_NUM; // Only care about params whose values are on the stack if (!ShadowParamVarInfo::mayNeedShadowCopy(varDsc)) @@ -450,7 +450,7 @@ void Compiler::gsParamsToShadows() unsigned int lclNum = tree->AsLclVarCommon()->GetLclNum(); unsigned int shadowLclNum = m_compiler->gsShadowVarInfo[lclNum].shadowCopy; - if (shadowLclNum != Compiler::ShadowParamVarInfo::NO_SHADOW_COPY) + if (shadowLclNum != BAD_VAR_NUM) { LclVarDsc* varDsc = m_compiler->lvaGetDesc(lclNum); assert(ShadowParamVarInfo::mayNeedShadowCopy(varDsc)); @@ -490,7 +490,7 @@ void Compiler::gsParamsToShadows() const LclVarDsc* varDsc = lvaGetDesc(lclNum); const unsigned shadowVarNum = gsShadowVarInfo[lclNum].shadowCopy; - if (shadowVarNum == ShadowParamVarInfo::NO_SHADOW_COPY) + if (shadowVarNum == BAD_VAR_NUM) { continue; } @@ -542,7 +542,7 @@ void Compiler::gsParamsToShadows() const LclVarDsc* varDsc = lvaGetDesc(lclNum); const unsigned shadowVarNum = gsShadowVarInfo[lclNum].shadowCopy; - if (shadowVarNum == ShadowParamVarInfo::NO_SHADOW_COPY) + if (shadowVarNum == BAD_VAR_NUM) { continue; }