From 0626684ad0b178840550e4d53903da8220192b5d Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 14 Nov 2018 11:41:04 -0800 Subject: [PATCH 1/2] Defer setting lvSingleDef until lvaMarkLocalVars Since this information isn't used early on in the jit, defer setting this bit until later. (a subsequent change will repurpose this bit and use it early too). --- src/jit/lclvars.cpp | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/src/jit/lclvars.cpp b/src/jit/lclvars.cpp index a8ffe7c8597d..bcab8cb43e34 100644 --- a/src/jit/lclvars.cpp +++ b/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 */ @@ -4155,6 +4138,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"); From 09b69f86b1c0a6ec24fe099fba9d341640a06693 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 14 Nov 2018 19:13:15 -0800 Subject: [PATCH 2/2] 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). --- src/jit/compiler.h | 5 ++- src/jit/flowgraph.cpp | 56 +++++++++++++++++++++++++++++++ src/jit/importer.cpp | 39 ++++++++++++++++++---- src/jit/lclvars.cpp | 78 +++++++++++++++++++++++-------------------- 4 files changed, 135 insertions(+), 43 deletions(-) diff --git a/src/jit/compiler.h b/src/jit/compiler.h index a60b22c454c2..4da6b4c4b8a2 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -359,8 +359,11 @@ class LclVarDsc #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/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index b2846ed7cfb0..55c465caa879 100644 --- a/src/jit/flowgraph.cpp +++ b/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) { @@ -22423,6 +22454,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/jit/importer.cpp b/src/jit/importer.cpp index 26aafb72b206..c22b373ba05e 100644 --- a/src/jit/importer.cpp +++ b/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); @@ -5686,9 +5689,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); } @@ -10192,6 +10197,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 @@ -10847,14 +10856,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); } @@ -12718,6 +12727,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()); } } @@ -13434,6 +13446,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); @@ -18737,6 +18753,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()); } @@ -18928,6 +18952,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/jit/lclvars.cpp b/src/jit/lclvars.cpp index bcab8cb43e34..9217fe7610d4 100644 --- a/src/jit/lclvars.cpp +++ b/src/jit/lclvars.cpp @@ -2689,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; }