From e9a3a6f93df3ae55baba41e04bf3ae0414eadca2 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 27 Sep 2021 14:19:20 +0200 Subject: [PATCH 1/2] Clean up fix for single-reg returned normalize-on-load vars This is a better fix for #58373 that changes the handling of this to happen in morph for all cases. We sometimes missed the insertion of necessary casts because we forgot to remove a GTF_DONT_CSE flag when folding an indirection. Fixing this leads to some new GT_CNS_DBL cases in lowering that hit an assert, but those cases should be correctly handled by the default case so just remove the assert. To get rid of some of the regressions I have allowed generating assertions when assigning struct fields from casts. It was unclear why this was not allowed in the first place. --- src/coreclr/jit/assertionprop.cpp | 11 ++------- src/coreclr/jit/lower.cpp | 39 +------------------------------ src/coreclr/jit/morph.cpp | 31 +++++++++++++----------- 3 files changed, 20 insertions(+), 61 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index e6334311c6aab9..9b252842978a9e 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -1547,21 +1547,14 @@ AssertionIndex Compiler::optCreateAssertion(GenTree* op1, // Try and see if we can make a subrange assertion. if (((assertionKind == OAK_SUBRANGE) || (assertionKind == OAK_EQUAL))) { - // Keep the casts on small struct fields. - // TODO-Review: why? - if (op2->OperIs(GT_CAST) && lvaTable[lclNum].lvIsStructField && lvaTable[lclNum].lvNormalizeOnLoad()) - { - goto DONE_ASSERTION; - } - if (optTryExtractSubrangeAssertion(op2, &assertion.op2.u2)) { assertion.op2.kind = O2K_SUBRANGE; assertion.assertionKind = OAK_SUBRANGE; } } - } // else // !helperCallArgs - } // if (op1->gtOper == GT_LCL_VAR) + } + } // // Are we making an IsType assertion? diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index c0eb2e264d00cc..aee712ae90d85a 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3371,16 +3371,6 @@ void Lowering::LowerRetStruct(GenTreeUnOp* ret) } break; -#if defined(TARGET_AMD64) || defined(TARGET_ARM64) || defined(TARGET_ARM) - case GT_CNS_DBL: - // Currently we are not promoting structs with a single float field, - // https://github.com/dotnet/runtime/issues/4323 - - // TODO-CQ: can improve `GT_CNS_DBL` handling for supported platforms, but - // because it is only x86 nowadays it is not worth it. - unreached(); -#endif - default: assert(varTypeIsEnregisterable(retVal)); if (varTypeUsesFloatReg(ret) != varTypeUsesFloatReg(retVal)) @@ -3414,24 +3404,7 @@ void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret) unsigned lclNum = lclVar->GetLclNum(); LclVarDsc* varDsc = comp->lvaGetDesc(lclNum); - bool replacedInLowering = false; - if (varDsc->CanBeReplacedWithItsField(comp)) - { - // We can replace the struct with its only field and keep the field on a register. - unsigned fieldLclNum = varDsc->lvFieldLclStart; - LclVarDsc* fieldDsc = comp->lvaGetDesc(fieldLclNum); - assert(varTypeIsSmallInt(fieldDsc->lvType)); // For a non-small type it had to be done in morph. - - lclVar->SetLclNum(fieldLclNum); - JITDUMP("Replacing an independently promoted local var V%02u with its only field V%02u for the return " - "[%06u]\n", - lclNum, fieldLclNum, comp->dspTreeID(ret)); - lclVar->ChangeType(fieldDsc->lvType); - lclNum = fieldLclNum; - varDsc = comp->lvaGetDesc(lclNum); - replacedInLowering = true; - } - else if (varDsc->lvPromoted) + if (varDsc->lvPromoted) { // TODO-1stClassStructs: We can no longer independently promote // or enregister this struct, since it is referenced as a whole. @@ -3469,16 +3442,6 @@ void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret) const var_types lclVarType = varDsc->GetRegisterType(lclVar); assert(lclVarType != TYP_UNDEF); - if (varDsc->lvNormalizeOnLoad() && replacedInLowering) - { - // For a normalize-on-load var that we replaced late we need to insert a cast - // as morph would typically be responsible for this. - GenTreeCast* cast = comp->gtNewCastNode(TYP_INT, lclVar, false, lclVarType); - ret->gtOp1 = cast; - BlockRange().InsertBefore(ret, cast); - ContainCheckCast(cast); - } - const var_types actualType = genActualType(lclVarType); lclVar->ChangeType(actualType); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index b58559b1bf43e4..626ebaed997c27 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -6000,7 +6000,11 @@ GenTree* Compiler::fgMorphLocalVar(GenTree* tree, bool forceRemorph) // TODO-CQ: fix the VNs for normalize-on-load locals and remove this quirk. bool isBoolQuirk = varType == TYP_BOOL; - // Assertion prop can tell us to omit adding a cast here. + // Assertion prop can tell us to omit adding a cast here. This is + // useful when the local is a small-typed parameter that is passed in a + // register: in that case, the ABI specifies that the upper bits might + // be invalid, but the assertion guarantees us that we have normalized + // when we wrote it. if (optLocalAssertionProp && !isBoolQuirk && optAssertionIsSubrange(tree, IntegralRange::ForType(varType), apFull) != NO_ASSERTION_INDEX) { @@ -11565,18 +11569,12 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) unsigned fieldLclNum = varDsc->lvFieldLclStart; LclVarDsc* fieldDsc = lvaGetDesc(fieldLclNum); - if (!varTypeIsSmallInt(fieldDsc->lvType)) - { - // TODO-CQ: support that substitution for small types without creating `CAST` node. - // When a small struct is returned in a register higher bits could be left in undefined - // state. - JITDUMP("Replacing an independently promoted local var V%02u with its only field " - "V%02u for " - "the return [%06u]\n", - lclVar->GetLclNum(), fieldLclNum, dspTreeID(tree)); - lclVar->SetLclNum(fieldLclNum); - lclVar->ChangeType(fieldDsc->lvType); - } + JITDUMP("Replacing an independently promoted local var V%02u with its only field " + "V%02u for " + "the return [%06u]\n", + lclVar->GetLclNum(), fieldLclNum, dspTreeID(tree)); + lclVar->SetLclNum(fieldLclNum); + lclVar->ChangeType(fieldDsc->lvType); } } } @@ -13929,7 +13927,12 @@ GenTree* Compiler::fgMorphRetInd(GenTreeUnOp* ret) DEBUG_DESTROY_NODE(ind); DEBUG_DESTROY_NODE(addr); ret->gtOp1 = lclVar; - return ret->gtGetOp1(); + // We use GTF_DONT_CSE as an "is under GT_ADDR" check. We can + // get rid of it now since the GT_RETURN node should never have + // its address taken. + assert((ret->gtFlags & GTF_DONT_CSE) == 0); + lclVar->gtFlags &= ~GTF_DONT_CSE; + return lclVar; } else if (!varDsc->lvDoNotEnregister) { From 5d839903420b835f3ae01e0f430fd46e2cae8869 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 27 Sep 2021 17:04:03 +0200 Subject: [PATCH 2/2] Allow cast assertions for normalize-on-load vars in global assertion prop --- src/coreclr/jit/assertionprop.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 9b252842978a9e..d0395283eb384a 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -2150,12 +2150,6 @@ AssertionIndex Compiler::optAssertionGenCast(GenTreeCast* cast) return NO_ASSERTION_INDEX; } - // This condition also exists to preverve previous behavior. - if (varDsc->lvIsStructField && varDsc->lvNormalizeOnLoad()) - { - return NO_ASSERTION_INDEX; - } - AssertionDsc assertion = {OAK_SUBRANGE}; assertion.op1.kind = O1K_LCLVAR; assertion.op1.vn = vnStore->VNConservativeNormalValue(lclVar->gtVNPair);