From 6c69793217d9dbe6301102d208f5e7c70cca655b Mon Sep 17 00:00:00 2001 From: petris Date: Wed, 26 Apr 2023 16:04:33 +0200 Subject: [PATCH 1/3] Set lvSingleDef for non TYP_REF locals --- src/coreclr/jit/fgbasic.cpp | 34 ++++++++++++------------ src/coreclr/jit/fgopt.cpp | 2 +- src/coreclr/jit/importer.cpp | 44 ++++++++++++++++++------------- src/coreclr/jit/importercalls.cpp | 7 +++-- 4 files changed, 47 insertions(+), 40 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index f0b87d933e17c8..5a8f5f9ff0bc56 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -2536,7 +2536,6 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed { LclVarDsc* lclDsc = lvaGetDesc(lclNum); assert(lclDsc->lvSingleDef == 0); - // could restrict this to TYP_REF lclDsc->lvSingleDef = !lclDsc->lvHasMultipleILStoreOp && !lclDsc->lvHasLdAddrOp; if (lclDsc->lvSingleDef) @@ -3494,19 +3493,20 @@ void Compiler::fgFindBasicBlocks() // This temp should already have the type of the return value. JITDUMP("\nInliner: re-using pre-existing spill temp V%02u\n", lvaInlineeReturnSpillTemp); - if (info.compRetType == TYP_REF) + // We may have co-opted an existing temp for the return spill. + // We likely assumed it was single-def at the time, but now + // we can see it has multiple definitions. + if ((fgReturnCount > 1) && (lvaTable[lvaInlineeReturnSpillTemp].lvSingleDef == 1)) { - // We may have co-opted an existing temp for the return spill. - // We likely assumed it was single-def at the time, but now - // we can see it has multiple definitions. - if ((fgReturnCount > 1) && (lvaTable[lvaInlineeReturnSpillTemp].lvSingleDef == 1)) + // Make sure it is no longer marked single def. This is only safe + // to do if we haven't ever updated the type. + if (info.compRetType == TYP_REF) { - // Make sure it is no longer marked single def. This is only safe - // to do if we haven't ever updated the type. assert(!lvaTable[lvaInlineeReturnSpillTemp].lvClassInfoUpdated); - JITDUMP("Marked return spill temp V%02u as NOT single def temp\n", lvaInlineeReturnSpillTemp); - lvaTable[lvaInlineeReturnSpillTemp].lvSingleDef = 0; } + + JITDUMP("Marked return spill temp V%02u as NOT single def temp\n", lvaInlineeReturnSpillTemp); + lvaTable[lvaInlineeReturnSpillTemp].lvSingleDef = 0; } } else @@ -3515,18 +3515,18 @@ void Compiler::fgFindBasicBlocks() lvaInlineeReturnSpillTemp = lvaGrabTemp(false DEBUGARG("Inline return value spill temp")); lvaTable[lvaInlineeReturnSpillTemp].lvType = info.compRetType; + // The return spill temp is single def only if the method has a single return block. + if (fgReturnCount == 1) + { + lvaTable[lvaInlineeReturnSpillTemp].lvSingleDef = 1; + JITDUMP("Marked return spill temp V%02u as a single def temp\n", lvaInlineeReturnSpillTemp); + } + // If the method returns a ref class, set the class of the spill temp // to the method's return value. We may update this later if it turns // out we can prove the method returns a more specific type. if (info.compRetType == TYP_REF) { - // The return spill temp is single def only if the method has a single return block. - if (fgReturnCount == 1) - { - lvaTable[lvaInlineeReturnSpillTemp].lvSingleDef = 1; - JITDUMP("Marked return spill temp V%02u as a single def temp\n", lvaInlineeReturnSpillTemp); - } - CORINFO_CLASS_HANDLE retClassHnd = impInlineInfo->inlineCandidateInfo->methInfo.args.retTypeClass; if (retClassHnd != nullptr) { diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 526cea6443d4ba..c6c840bceb1bfa 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -1453,7 +1453,7 @@ PhaseStatus Compiler::fgPostImportationCleanup() { LclVarDsc* returnSpillVarDsc = lvaGetDesc(lvaInlineeReturnSpillTemp); - if (returnSpillVarDsc->lvSingleDef) + if (returnSpillVarDsc->lvType == TYP_REF && returnSpillVarDsc->lvSingleDef) { lvaUpdateClass(lvaInlineeReturnSpillTemp, retExprClassHnd, impInlineInfo->retExprClassHndIsExact); } diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index e7d03fbc9c2369..f762c0d796514a 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -1942,14 +1942,18 @@ bool Compiler::impSpillStackEntry(unsigned level, /* Assign the spilled entry to the temp */ impAssignTempGen(tnum, tree, verCurrentState.esStack[level].seTypeInfo.GetClassHandle(), level); - // If temp is newly introduced and a ref type, grab what type info we can. - if (isNewTemp && (lvaTable[tnum].lvType == TYP_REF)) + if (isNewTemp) { assert(lvaTable[tnum].lvSingleDef == 0); lvaTable[tnum].lvSingleDef = 1; JITDUMP("Marked V%02u as a single def temp\n", tnum); - CORINFO_CLASS_HANDLE stkHnd = verCurrentState.esStack[level].seTypeInfo.GetClassHandle(); - lvaSetClass(tnum, tree, stkHnd); + + // If temp is newly introduced and a ref type, grab what type info we can. + if (lvaTable[tnum].lvType == TYP_REF) + { + CORINFO_CLASS_HANDLE stkHnd = verCurrentState.esStack[level].seTypeInfo.GetClassHandle(); + lvaSetClass(tnum, tree, stkHnd); + } // If we're assigning a GT_RET_EXPR, note the temp over on the call, // so the inliner can use it in case it needs a return spill temp. @@ -8453,12 +8457,12 @@ void Compiler::impImportBlockCode(BasicBlock* block) impAssignTempGen(tmpNum, op1, tiRetVal.GetClassHandle(), CHECK_SPILL_ALL); var_types type = genActualType(lvaTable[tmpNum].TypeGet()); + assert(lvaTable[tmpNum].lvSingleDef == 0); + lvaTable[tmpNum].lvSingleDef = 1; + JITDUMP("Marked V%02u as a single def local\n", tmpNum); // Propagate type info to the temp from the stack and the original tree if (type == TYP_REF) { - assert(lvaTable[tmpNum].lvSingleDef == 0); - lvaTable[tmpNum].lvSingleDef = 1; - JITDUMP("Marked V%02u as a single def local\n", tmpNum); lvaSetClass(tmpNum, tree, tiRetVal.GetClassHandle()); } @@ -13662,19 +13666,19 @@ unsigned Compiler::impInlineFetchLocal(unsigned lclNum DEBUGARG(const char* reas lvaTable[tmpNum].lvHasILStoreOp = inlineeLocal.lclHasStlocOp; lvaTable[tmpNum].lvHasMultipleILStoreOp = inlineeLocal.lclHasMultipleStlocOp; + assert(lvaTable[tmpNum].lvSingleDef == 0); + + lvaTable[tmpNum].lvSingleDef = !inlineeLocal.lclHasMultipleStlocOp && !inlineeLocal.lclHasLdlocaOp; + if (lvaTable[tmpNum].lvSingleDef) + { + JITDUMP("Marked V%02u as a single def temp\n", tmpNum); + } + // Copy over class handle for ref types. Note this may be a // shared type -- someday perhaps we can get the exact // signature and pass in a more precise type. if (lclTyp == TYP_REF) { - assert(lvaTable[tmpNum].lvSingleDef == 0); - - lvaTable[tmpNum].lvSingleDef = !inlineeLocal.lclHasMultipleStlocOp && !inlineeLocal.lclHasLdlocaOp; - if (lvaTable[tmpNum].lvSingleDef) - { - JITDUMP("Marked V%02u as a single def temp\n", tmpNum); - } - lvaSetClass(tmpNum, inlineeLocal.lclVerTypeInfo.GetClassHandleForObjRef()); } @@ -13859,6 +13863,13 @@ GenTree* Compiler::impInlineFetchArg(unsigned lclNum, InlArgInfo* inlArgInfo, In lvaTable[tmpNum].lvType = lclTyp; + if (!argCanBeModified) + { + assert(lvaTable[tmpNum].lvSingleDef == 0); + lvaTable[tmpNum].lvSingleDef = 1; + JITDUMP("Marked V%02u as a single def temp\n", tmpNum); + } + // For ref types, determine the type of the temp. if (lclTyp == TYP_REF) { @@ -13867,9 +13878,6 @@ GenTree* Compiler::impInlineFetchArg(unsigned lclNum, InlArgInfo* inlArgInfo, In // If the arg can't be modified in the method // body, use the type of the value, if // known. Otherwise, use the declared type. - assert(lvaTable[tmpNum].lvSingleDef == 0); - lvaTable[tmpNum].lvSingleDef = 1; - JITDUMP("Marked V%02u as a single def temp\n", tmpNum); lvaSetClass(tmpNum, argNode, lclInfo.lclVerTypeInfo.GetClassHandleForObjRef()); } else diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 0d6cd2aa8fb807..a446dbe546d3a6 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5476,12 +5476,11 @@ class SpillRetExprHelper comp->impAssignTempGen(tmp, retExpr, (unsigned)Compiler::CHECK_SPILL_NONE); *pRetExpr = comp->gtNewLclvNode(tmp, retExpr->TypeGet()); + assert(comp->lvaTable[tmp].lvSingleDef == 0); + comp->lvaTable[tmp].lvSingleDef = 1; + JITDUMP("Marked V%02u as a single def temp\n", tmp); if (retExpr->TypeGet() == TYP_REF) { - assert(comp->lvaTable[tmp].lvSingleDef == 0); - comp->lvaTable[tmp].lvSingleDef = 1; - JITDUMP("Marked V%02u as a single def temp\n", tmp); - bool isExact = false; bool isNonNull = false; CORINFO_CLASS_HANDLE retClsHnd = comp->gtGetClassHandle(retExpr, &isExact, &isNonNull); From c40796201b7ef9be43ecdb3b00dd04abb8f5ab43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Sat, 29 Apr 2023 20:56:39 +0200 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Andy Ayers --- src/coreclr/jit/fgopt.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index c6c840bceb1bfa..6dc05ed97dfc38 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -1453,7 +1453,7 @@ PhaseStatus Compiler::fgPostImportationCleanup() { LclVarDsc* returnSpillVarDsc = lvaGetDesc(lvaInlineeReturnSpillTemp); - if (returnSpillVarDsc->lvType == TYP_REF && returnSpillVarDsc->lvSingleDef) + if ((returnSpillVarDsc->lvType == TYP_REF) && returnSpillVarDsc->lvSingleDef) { lvaUpdateClass(lvaInlineeReturnSpillTemp, retExprClassHnd, impInlineInfo->retExprClassHndIsExact); } From 7699bd1704abae08341ea06d6df4af15c7e6dce4 Mon Sep 17 00:00:00 2001 From: petris Date: Sun, 30 Apr 2023 19:25:57 +0200 Subject: [PATCH 3/3] Rewrite an if --- src/coreclr/jit/importer.cpp | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index f762c0d796514a..ded3be3dcc5bb1 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -13863,24 +13863,21 @@ GenTree* Compiler::impInlineFetchArg(unsigned lclNum, InlArgInfo* inlArgInfo, In lvaTable[tmpNum].lvType = lclTyp; + // If arg can't be modified, mark it as single def. + // For ref types, determine the class of the arg temp. if (!argCanBeModified) { assert(lvaTable[tmpNum].lvSingleDef == 0); lvaTable[tmpNum].lvSingleDef = 1; JITDUMP("Marked V%02u as a single def temp\n", tmpNum); - } - - // For ref types, determine the type of the temp. - if (lclTyp == TYP_REF) - { - if (!argCanBeModified) + if (lclTyp == TYP_REF) { - // If the arg can't be modified in the method - // body, use the type of the value, if - // known. Otherwise, use the declared type. lvaSetClass(tmpNum, argNode, lclInfo.lclVerTypeInfo.GetClassHandleForObjRef()); } - else + } + else + { + if (lclTyp == TYP_REF) { // Arg might be modified, use the declared type of // the argument.