From d81a4add0c07ed796717a7fece737490792b85dd Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 29 Nov 2018 00:32:55 -0800 Subject: [PATCH] Track single def locals in importer (dotnet/coreclr#21251) * Defer setting lvSingleDef until lvaMarkLocalVars Since this information isn't used early on in the jit, defer setting this bit until later. * Track single-def locals in the importer and use this information to enable type updates during late devirtualization. This gets some cases where the exact type materializes late (perhaps via a chain of copies). Commit migrated from https://github.com/dotnet/coreclr/commit/40bf810b5cb83ba45008f9f8c12a4e3d46eb6832 --- src/coreclr/src/jit/compiler.h | 5 +- src/coreclr/src/jit/flowgraph.cpp | 56 ++++++++++++++++++++ src/coreclr/src/jit/importer.cpp | 39 +++++++++++--- src/coreclr/src/jit/lclvars.cpp | 105 ++++++++++++++++++-------------------- 4 files changed, 142 insertions(+), 63 deletions(-) diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 3ac8b9c..986f0ab 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -359,8 +359,11 @@ public: #if OPT_BOOL_OPS unsigned char lvIsBoolean : 1; // set if variable is boolean #endif + unsigned char lvSingleDef : 1; // variable has a single def + // before lvaMarkLocalVars: identifies ref type locals that can get type updates + // after lvaMarkLocalVars: identifies locals that are suitable for optAddCopies + #if ASSERTION_PROP - unsigned char lvSingleDef : 1; // variable has a single def unsigned char lvDisqualify : 1; // variable is no longer OK for add copy optimization unsigned char lvVolatileHint : 1; // hint for AssertionProp #endif diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp index 57948cf..41a88d9 100644 --- a/src/coreclr/src/jit/flowgraph.cpp +++ b/src/coreclr/src/jit/flowgraph.cpp @@ -4944,6 +4944,34 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed { fgAdjustForAddressExposedOrWrittenThis(); } + + // Now that we've seen the IL, set lvSingleDef for root method + // locals. + // + // We could also do this for root method arguments but single-def + // arguments are set by the caller and so we don't know anything + // about the possible values or types. + // + // For inlinees we do this over in impInlineFetchLocal and + // impInlineFetchArg (here args are included as we somtimes get + // new information about the types of inlinee args). + if (!isInlining) + { + const unsigned firstLcl = info.compArgsCount; + const unsigned lastLcl = firstLcl + info.compMethodInfo->locals.numArgs; + for (unsigned lclNum = firstLcl; lclNum < lastLcl; lclNum++) + { + LclVarDsc* lclDsc = lvaGetDesc(lclNum); + assert(lclDsc->lvSingleDef == 0); + // could restrict this to TYP_REF + lclDsc->lvSingleDef = !lclDsc->lvHasMultipleILStoreOp && !lclDsc->lvHasLdAddrOp; + + if (lclDsc->lvSingleDef) + { + JITDUMP("Marked V%02u as a single def local\n", lclNum); + } + } + } } #ifdef _PREFAST_ @@ -5876,6 +5904,9 @@ void Compiler::fgFindBasicBlocks() // out we can prove the method returns a more specific type. if (info.compRetType == TYP_REF) { + lvaTable[lvaInlineeReturnSpillTemp].lvSingleDef = 1; + JITDUMP("Marked V%02u as a single def temp\n", lvaInlineeReturnSpillTemp); + CORINFO_CLASS_HANDLE retClassHnd = impInlineInfo->inlineCandidateInfo->methInfo.args.retTypeClass; if (retClassHnd != nullptr) { @@ -22442,6 +22473,31 @@ Compiler::fgWalkResult Compiler::fgLateDevirtualization(GenTree** pTree, fgWalkD comp->impDevirtualizeCall(call, &method, &methodFlags, &context, nullptr); } } + else if (tree->OperGet() == GT_ASG) + { + // If we're assigning to a ref typed local that has one definition, + // we may be able to sharpen the type for the local. + GenTree* lhs = tree->gtGetOp1()->gtEffectiveVal(); + + if ((lhs->OperGet() == GT_LCL_VAR) && (lhs->TypeGet() == TYP_REF)) + { + const unsigned lclNum = lhs->gtLclVarCommon.gtLclNum; + LclVarDsc* lcl = comp->lvaGetDesc(lclNum); + + if (lcl->lvSingleDef) + { + GenTree* rhs = tree->gtGetOp2(); + bool isExact = false; + bool isNonNull = false; + CORINFO_CLASS_HANDLE newClass = comp->gtGetClassHandle(rhs, &isExact, &isNonNull); + + if (newClass != NO_CLASS_HANDLE) + { + comp->lvaUpdateClass(lclNum, newClass, isExact); + } + } + } + } return WALK_CONTINUE; } diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp index 371d277..cefb2fa 100644 --- a/src/coreclr/src/jit/importer.cpp +++ b/src/coreclr/src/jit/importer.cpp @@ -2255,6 +2255,9 @@ bool Compiler::impSpillStackEntry(unsigned level, // If temp is newly introduced and a ref type, grab what type info we can. if (isNewTemp && (lvaTable[tnum].lvType == TYP_REF)) { + 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); @@ -6052,9 +6055,11 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) { // When optimizing, use a new temp for each box operation // since we then know the exact class of the box temp. - impBoxTemp = lvaGrabTemp(true DEBUGARG("Single-def Box Helper")); - lvaTable[impBoxTemp].lvType = TYP_REF; - const bool isExact = true; + impBoxTemp = lvaGrabTemp(true DEBUGARG("Single-def Box Helper")); + lvaTable[impBoxTemp].lvType = TYP_REF; + lvaTable[impBoxTemp].lvSingleDef = 1; + JITDUMP("Marking V%02u as a single def local\n", impBoxTemp); + const bool isExact = true; lvaSetClass(impBoxTemp, pResolvedToken->hClass, isExact); } @@ -10559,6 +10564,10 @@ GenTree* Compiler::impCastClassOrIsInstToTree(GenTree* op1, // // See also gtGetHelperCallClassHandle where we make the same // determination for the helper call variants. + LclVarDsc* lclDsc = lvaGetDesc(tmp); + assert(lclDsc->lvSingleDef == 0); + lclDsc->lvSingleDef = 1; + JITDUMP("Marked V%02u as a single def temp\n", tmp); lvaSetClass(tmp, pResolvedToken->hClass); return gtNewLclvNode(tmp, TYP_REF); #endif @@ -11214,14 +11223,14 @@ void Compiler::impImportBlockCode(BasicBlock* block) // We should have seen a stloc in our IL prescan. assert(lvaTable[lclNum].lvHasILStoreOp); - const bool isSingleILStoreLocal = - !lvaTable[lclNum].lvHasMultipleILStoreOp && !lvaTable[lclNum].lvHasLdAddrOp; + // Is there just one place this local is defined? + const bool isSingleDefLocal = lvaTable[lclNum].lvSingleDef; // Conservative check that there is just one // definition that reaches this store. const bool hasSingleReachingDef = (block->bbStackDepthOnEntry() == 0); - if (isSingleILStoreLocal && hasSingleReachingDef) + if (isSingleDefLocal && hasSingleReachingDef) { lvaUpdateClass(lclNum, op1, clsHnd); } @@ -13085,6 +13094,9 @@ void Compiler::impImportBlockCode(BasicBlock* block) // 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()); } } @@ -13801,6 +13813,10 @@ void Compiler::impImportBlockCode(BasicBlock* block) // without exhaustive walk over all expressions. impAssignTempGen(lclNum, op1, (unsigned)CHECK_SPILL_NONE); + + assert(lvaTable[lclNum].lvSingleDef == 0); + lvaTable[lclNum].lvSingleDef = 1; + JITDUMP("Marked V%02u as a single def local\n", lclNum); lvaSetClass(lclNum, resolvedToken.hClass, true /* is Exact */); newObjThisPtr = gtNewLclvNode(lclNum, TYP_REF); @@ -19105,6 +19121,14 @@ unsigned Compiler::impInlineFetchLocal(unsigned lclNum DEBUGARG(const char* reas // 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()); } @@ -19296,6 +19320,9 @@ 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, argInfo.argNode, lclInfo.lclVerTypeInfo.GetClassHandleForObjRef()); } else diff --git a/src/coreclr/src/jit/lclvars.cpp b/src/coreclr/src/jit/lclvars.cpp index a8ffe7c..9217fe7 100644 --- a/src/coreclr/src/jit/lclvars.cpp +++ b/src/coreclr/src/jit/lclvars.cpp @@ -387,11 +387,7 @@ void Compiler::lvaInitThisPtr(InitVarDscInfo* varDscInfo) if (!info.compIsStatic) { varDsc->lvIsParam = 1; -#if ASSERTION_PROP - varDsc->lvSingleDef = 1; -#endif - - varDsc->lvIsPtr = 1; + varDsc->lvIsPtr = 1; lvaArg0Var = info.compThisArg = varDscInfo->varNum; noway_assert(info.compThisArg == 0); @@ -474,9 +470,7 @@ void Compiler::lvaInitRetBuffArg(InitVarDscInfo* varDscInfo) varDsc->lvType = TYP_BYREF; varDsc->lvIsParam = 1; varDsc->lvIsRegArg = 1; -#if ASSERTION_PROP - varDsc->lvSingleDef = 1; -#endif + if (hasFixedRetBuffReg()) { varDsc->lvArgReg = theFixedRetBuffReg(); @@ -564,9 +558,6 @@ void Compiler::lvaInitUserArgs(InitVarDscInfo* varDscInfo) CorInfoTypeWithMod corInfoType = info.compCompHnd->getArgType(&info.compMethodInfo->args, argLst, &typeHnd); varDsc->lvIsParam = 1; -#if ASSERTION_PROP - varDsc->lvSingleDef = 1; -#endif lvaInitVarDsc(varDsc, varDscInfo->varNum, strip(corInfoType), typeHnd, argLst, &info.compMethodInfo->args); @@ -1046,11 +1037,7 @@ void Compiler::lvaInitGenericsCtxt(InitVarDscInfo* varDscInfo) LclVarDsc* varDsc = varDscInfo->varDsc; varDsc->lvIsParam = 1; -#if ASSERTION_PROP - varDsc->lvSingleDef = 1; -#endif - - varDsc->lvType = TYP_I_IMPL; + varDsc->lvType = TYP_I_IMPL; if (varDscInfo->canEnreg(TYP_I_IMPL)) { @@ -1112,10 +1099,6 @@ void Compiler::lvaInitVarArgsHandle(InitVarDscInfo* varDscInfo) // that other problems are fixed. lvaSetVarAddrExposed(varDscInfo->varNum); -#if ASSERTION_PROP - varDsc->lvSingleDef = 1; -#endif - if (varDscInfo->canEnreg(TYP_I_IMPL)) { /* Another register argument */ @@ -2706,54 +2689,60 @@ void Compiler::lvaUpdateClass(unsigned varNum, CORINFO_CLASS_HANDLE clsHnd, bool // We should already have a class assert(varDsc->lvClassHnd != nullptr); -#if defined(DEBUG) + // We should only be updating classes for single-def locals. + assert(varDsc->lvSingleDef); - // In general we only expect one update per local var. However if - // a block is re-imported and that block has the only STLOC for - // the var, we may see multiple updates. All subsequent updates - // should agree on the type, since reimportation is triggered by - // type mismatches for things other than ref types. - if (varDsc->lvClassInfoUpdated) - { - assert(varDsc->lvClassHnd == clsHnd); - assert(varDsc->lvClassIsExact == isExact); - } + // Now see if we should update. + // + // New information may not always be "better" so do some + // simple analysis to decide if the update is worthwhile. + const bool isNewClass = (clsHnd != varDsc->lvClassHnd); + bool shouldUpdate = false; - // This counts as an update, even if nothing changes. - varDsc->lvClassInfoUpdated = true; + // Are we attempting to update the class? Only check this when we have + // an new type and the existing class is inexact... we should not be + // updating exact classes. + if (!varDsc->lvClassIsExact && isNewClass) + { + // Todo: improve this analysis by adding a new jit interface method + DWORD newAttrs = info.compCompHnd->getClassAttribs(clsHnd); + DWORD oldAttrs = info.compCompHnd->getClassAttribs(varDsc->lvClassHnd); -#endif // defined(DEBUG) + // Avoid funny things with __Canon by only merging if both shared or both unshared + if ((newAttrs & CORINFO_FLG_SHAREDINST) == (oldAttrs & CORINFO_FLG_SHAREDINST)) + { + // If we merge types and we get back the old class, the new class is more + // specific and we should update to it. + CORINFO_CLASS_HANDLE mergeClass = info.compCompHnd->mergeClasses(clsHnd, varDsc->lvClassHnd); - // If previous type was exact, there is nothing to update. Would - // like to verify new type is compatible but can't do this yet. - if (varDsc->lvClassIsExact) - { - return; + if (mergeClass == varDsc->lvClassHnd) + { + shouldUpdate = true; + } + } + else if ((newAttrs & CORINFO_FLG_SHAREDINST) == 0) + { + // Update if we go from shared to unshared + shouldUpdate = true; + } } - - // Are we updating the type? - if (varDsc->lvClassHnd != clsHnd) + // Else are we attempting to update exactness? + else if (isExact && !varDsc->lvClassIsExact && !isNewClass) { - JITDUMP("\nlvaUpdateClass: Updating class for V%02i from (%p) %s to (%p) %s %s\n", varNum, - dspPtr(varDsc->lvClassHnd), info.compCompHnd->getClassName(varDsc->lvClassHnd), dspPtr(clsHnd), - info.compCompHnd->getClassName(clsHnd), isExact ? " [exact]" : ""); - - varDsc->lvClassHnd = clsHnd; - varDsc->lvClassIsExact = isExact; - return; + shouldUpdate = true; } - // Class info matched. Are we updating exactness? - if (isExact) - { - JITDUMP("\nlvaUpdateClass: Updating class for V%02i (%p) %s to be exact\n", varNum, dspPtr(varDsc->lvClassHnd), - info.compCompHnd->getClassName(varDsc->lvClassHnd)); + JITDUMP("\nlvaUpdateClass:%s Updating class for V%02u from (%p) %s%s to (%p) %s%s\n", varNum, + shouldUpdate ? "" : " NOT", dspPtr(varDsc->lvClassHnd), info.compCompHnd->getClassName(varDsc->lvClassHnd), + varDsc->lvClassIsExact ? " [exact]" : "", dspPtr(clsHnd), info.compCompHnd->getClassName(clsHnd), + isExact ? " [exact]" : ""); + if (shouldUpdate) + { + varDsc->lvClassHnd = clsHnd; varDsc->lvClassIsExact = isExact; - return; } - // Else we have the same handle and (in)exactness as before. Do nothing. return; } @@ -4155,6 +4144,10 @@ void Compiler::lvaComputeRefCounts(bool isRecompute, bool setSlotNumbers) { varDsc->lvSlotNum = lclNum; } + + // Set initial value for lvSingleDef for explicit and implicit + // argument locals as they are "defined" on entry. + varDsc->lvSingleDef = varDsc->lvIsParam; } JITDUMP("\n*** lvaComputeRefCounts -- explicit counts ***\n"); -- 2.7.4