From a193cb5ca0ac1ec5dd694a94167aa3e4cc89d613 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Mon, 1 May 2023 19:54:00 +0200 Subject: [PATCH] Set lvSingleDef for non TYP_REF locals (#85398) * Set lvSingleDef for non TYP_REF locals * Apply suggestions from code review Co-authored-by: Andy Ayers * Rewrite an if --------- Co-authored-by: Andy Ayers --- src/coreclr/jit/fgbasic.cpp | 34 ++++++++++++------------ src/coreclr/jit/fgopt.cpp | 2 +- src/coreclr/jit/importer.cpp | 55 +++++++++++++++++++++------------------ src/coreclr/jit/importercalls.cpp | 7 +++-- 4 files changed, 51 insertions(+), 47 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index f0b87d9..5a8f5f9 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 526cea6..6dc05ed 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 75f71b5..fca9e62 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -1862,14 +1862,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. @@ -8373,12 +8377,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()); } @@ -13582,19 +13586,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()); } @@ -13779,20 +13783,21 @@ GenTree* Compiler::impInlineFetchArg(unsigned lclNum, InlArgInfo* inlArgInfo, In lvaTable[tmpNum].lvType = lclTyp; - // For ref types, determine the type of the temp. - if (lclTyp == TYP_REF) + // If arg can't be modified, mark it as single def. + // For ref types, determine the class of the arg temp. + if (!argCanBeModified) { - if (!argCanBeModified) + assert(lvaTable[tmpNum].lvSingleDef == 0); + lvaTable[tmpNum].lvSingleDef = 1; + JITDUMP("Marked V%02u as a single def temp\n", tmpNum); + 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. - 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 + } + else + { + if (lclTyp == TYP_REF) { // Arg might be modified, use the declared type of // the argument. diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 1cd5cc5..e570c99 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5477,12 +5477,11 @@ private: 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); -- 2.7.4