From 6e33c4448a0f0e186ddef2b40f2681f0faee6f9e Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 2 Aug 2017 19:41:40 -0700 Subject: [PATCH 1/2] JIT: modify box/unbox/isinst/castclass expansions for fast jitting When the jit is generating code in debug/minopts/rare-block modes, we'd prefer it to generate code more quickly and worry less about overall generated code performance. Generally speaking smaller intermediate and final code should correlate well with faster jitting. This change alters the expansions of box, unbox, isinst, and castclass when generating code for minopts, debug, or in rarely run blocks. In such modes the jit estimates whether an inline sequence or general helper call would result in more compact code, and then chooses the smaller sequence. This reduces generated code size around 2.5% in a variety of scenarios, and roughly translates to a 1.5% improvement in time spent jitting. Similar strategies can be applied to other complex operations during importation. That work is forthcoming. --- src/jit/importer.cpp | 230 +++++++++++++++++++++++++--------------- src/jit/valuenum.cpp | 5 + src/jit/valuenumfuncs.h | 1 + 3 files changed, 151 insertions(+), 85 deletions(-) diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index d529bb2fe9da..4c00c5913e37 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -5182,33 +5182,79 @@ GenTreePtr Compiler::impImportLdvirtftn(GenTreePtr thisPtr, return gtNewHelperCallNode(CORINFO_HELP_VIRTUAL_FUNC_PTR, TYP_I_IMPL, GTF_EXCEPT, helpArgs); } -/***************************************************************************** - * - * Build and import a box node - */ +//------------------------------------------------------------------------ +// impImportAndPushBox: build and import a value-type box +// +// Arguments: +// pResolvedToken - resolved token from the box operation +// +// Return Value: +// None. +// +// Side Effects: +// The value to be boxed is popped from the stack, and a tree for +// the boxed value is pushed. This method may create upstream +// statements, spill side effecting trees, and create new temps. +// +// If importing an inlinee, we may also discover the inline must +// fail. If so there is no new value pushed on the stack. Callers +// should use CompDoNotInline after calling this method to see if +// ongoing importation should be aborted. +// +// Notes: +// Boxing of ref classes results in the same value as the value on +// the top of the stack, so is handled inline in impImportBlockCode +// for the CEE_BOX case. Only value or primitive type boxes make it +// here. +// +// Boxing for nullable types is done via a helper call; boxing +// of other value types is expanded inline or handled via helper +// call, depending on the jit's codegen mode. +// +// When the jit is operating in size and time constrained modes, +// using a helper call here can save jit time and code size. But it +// also may inhibit cleanup optimizations that could have also had a +// even greater benefit effect on code size and jit time. An optimal +// strategy may need to peek ahead and see if it is easy to tell how +// the box is being used. For now, we defer. void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) { - // Get the tree for the type handle for the boxed object. In the case - // of shared generic code or ngen'd code this might be an embedded - // computation. - // Note we can only box do it if the class construtor has been called - // We can always do it on primitive types - - GenTreePtr op1 = nullptr; - GenTreePtr op2 = nullptr; - var_types lclTyp; - + // Spill any special side effects impSpillSpecialSideEff(); - // Now get the expression to box from the stack. + // Get get the expression to box from the stack. + GenTreePtr op1 = nullptr; + GenTreePtr op2 = nullptr; StackEntry se = impPopStack(); CORINFO_CLASS_HANDLE operCls = se.seTypeInfo.GetClassHandle(); GenTreePtr exprToBox = se.val; + // Look at what helper we should use. CorInfoHelpFunc boxHelper = info.compCompHnd->getBoxHelper(pResolvedToken->hClass); - if (boxHelper == CORINFO_HELP_BOX) + + // Determine what expansion to prefer. + // + // In size/time/debuggable constrained modes, the helper call + // expansion for box is generally smaller and is preferred, unless + // the value to box is a struct that comes from a call. In that + // case the call can construct its return value directly into the + // box payload, saving possibly some up-front zeroing. + // + // Currently primitive type boxes always get inline expanded. We may + // want to do the same for small structs if they don't come from + // calls and don't have GC pointers, since explicitly copying such + // structs is cheap. + JITDUMP("\nCompiler::impImportAndPushBox -- handling BOX(value class) via"); + bool canExpandInline = (boxHelper == CORINFO_HELP_BOX); + bool optForSize = !exprToBox->IsCall() && (operCls != nullptr) && + (opts.compDbgCode || opts.MinOpts() || compCurBB->isRunRarely()); + bool expandInline = canExpandInline && !optForSize; + + if (expandInline) { + JITDUMP(" inline allocate/copy sequence\n"); + // we are doing 'normal' boxing. This means that we can inline the box operation // Box(expr) gets morphed into // temp = new(clsHnd) @@ -5252,7 +5298,9 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) // Ensure that the value class is restored op2 = impTokenToHandle(pResolvedToken, nullptr, TRUE /* mustRestoreHandle */); if (op2 == nullptr) - { // compDonotInline() + { + // We must be backing out of an inline. + assert(compDonotInline()); return; } @@ -5278,7 +5326,7 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) } else { - lclTyp = exprToBox->TypeGet(); + var_types lclTyp = exprToBox->TypeGet(); if (lclTyp == TYP_BYREF) { lclTyp = TYP_I_IMPL; @@ -5325,12 +5373,17 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) } else { - // Don't optimize, just call the helper and be done with it + JITDUMP(" helper call because: %s\n", !canExpandInline ? "optimizing for size" : "nullable"); + assert(operCls != nullptr); + // Don't optimize, just call the helper and be done with it + // // Ensure that the value class is restored op2 = impTokenToHandle(pResolvedToken, nullptr, TRUE /* mustRestoreHandle */); if (op2 == nullptr) - { // compDonotInline() + { + // We must be backing out of an inline. + assert(compDonotInline()); return; } @@ -9406,78 +9459,84 @@ var_types Compiler::impGetByRefResultType(genTreeOps oper, bool fUnsigned, GenTr return type; } -/***************************************************************************** - * Casting Helper Function to service both CEE_CASTCLASS and CEE_ISINST - * - * typeRef contains the token, op1 to contain the value being cast, - * and op2 to contain code that creates the type handle corresponding to typeRef - * isCastClass = true means CEE_CASTCLASS, false means CEE_ISINST - */ +//------------------------------------------------------------------------ +// impCastClassOrIsInstToTree: build and import castclass/isinst +// +// Arguments: +// op1 - value to cast +// op2 - type handle for type to cast to +// pResolvedToken - resolved token from the cast operation +// isCastClass - true if this is castclass, false means isinst +// +// Return Value: +// Tree representing the cast +// +// Notes: +// May expand into a series of runtime checks or a helper call. + GenTreePtr Compiler::impCastClassOrIsInstToTree(GenTreePtr op1, GenTreePtr op2, CORINFO_RESOLVED_TOKEN* pResolvedToken, bool isCastClass) { - bool expandInline; - assert(op1->TypeGet() == TYP_REF); - CorInfoHelpFunc helper = info.compCompHnd->getCastingHelper(pResolvedToken, isCastClass); + // Optimistically assume the jit should expand this as an inline test + bool shouldExpandInline = true; - if (isCastClass) + // Profitability check. + // + // Don't bother with inline expansion when jit is trying to + // generate code quickly, or the cast is in code that won't run very + // often, or the method already is pretty big. + if (compCurBB->isRunRarely() || opts.compDbgCode || opts.MinOpts()) { - // We only want to expand inline the normal CHKCASTCLASS helper; - expandInline = (helper == CORINFO_HELP_CHKCASTCLASS); + // not worth the code expansion if jitting fast or in a rarely run block + shouldExpandInline = false; } - else + else if ((op1->gtFlags & GTF_GLOB_EFFECT) && lvaHaveManyLocals()) { - if (helper == CORINFO_HELP_ISINSTANCEOFCLASS) - { - // Get the Class Handle abd class attributes for the type we are casting to - // - DWORD flags = info.compCompHnd->getClassAttribs(pResolvedToken->hClass); - - // - // If the class handle is marked as final we can also expand the IsInst check inline - // - expandInline = ((flags & CORINFO_FLG_FINAL) != 0); - - // - // But don't expand inline these two cases - // - if (flags & CORINFO_FLG_MARSHAL_BYREF) - { - expandInline = false; - } - else if (flags & CORINFO_FLG_CONTEXTFUL) - { - expandInline = false; - } - } - else - { - // - // We can't expand inline any other helpers - // - expandInline = false; - } + // not worth creating an untracked local variable + shouldExpandInline = false; } - if (expandInline) + // Pessimistically assume the jit cannot expand this as an inline test + bool canExpandInline = false; + const CorInfoHelpFunc helper = info.compCompHnd->getCastingHelper(pResolvedToken, isCastClass); + + // Legality check. + // + // Not all classclass/isinst operations can be inline expanded. + // Check legality only if an inline expansion is desirable. + if (shouldExpandInline) { - if (compCurBB->isRunRarely()) + if (isCastClass) { - expandInline = false; // not worth the code expansion in a rarely run block + // Jit can only inline expand the normal CHKCASTCLASS helper. + canExpandInline = (helper == CORINFO_HELP_CHKCASTCLASS); } - - if ((op1->gtFlags & GTF_GLOB_EFFECT) && lvaHaveManyLocals()) + else { - expandInline = false; // not worth creating an untracked local variable + if (helper == CORINFO_HELP_ISINSTANCEOFCLASS) + { + // Check the class attributes. + DWORD flags = info.compCompHnd->getClassAttribs(pResolvedToken->hClass); + + // If the class is final and is not marshal byref or + // contextful, the jit can expand the IsInst check inline. + DWORD flagsMask = CORINFO_FLG_FINAL | CORINFO_FLG_MARSHAL_BYREF | CORINFO_FLG_CONTEXTFUL; + canExpandInline = ((flags & flagsMask) == CORINFO_FLG_FINAL); + } } } + const bool expandInline = canExpandInline && shouldExpandInline; + if (!expandInline) { + JITDUMP("\nExpanding %s as call because %s\n", isCastClass ? "castclass" : "isinst", + canExpandInline ? "want smaller code or faster jitting" : "inline expansion not legal"); + // If we CSE this class handle we prevent assertionProp from making SubType assertions // so instead we force the CSE logic to not consider CSE-ing this class handle. // @@ -9486,6 +9545,8 @@ GenTreePtr Compiler::impCastClassOrIsInstToTree(GenTreePtr op1, return gtNewHelperCallNode(helper, TYP_REF, 0, gtNewArgList(op2, op1)); } + JITDUMP("\nExpanding %s inline\n", isCastClass ? "castclass" : "isinst"); + impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("bubbling QMark2")); GenTreePtr temp; @@ -9539,9 +9600,9 @@ GenTreePtr Compiler::impCastClassOrIsInstToTree(GenTreePtr op1, // // use the special helper that skips the cases checked by our inlined cast // - helper = CORINFO_HELP_CHKCASTCLASS_SPECIAL; + const CorInfoHelpFunc specialHelper = CORINFO_HELP_CHKCASTCLASS_SPECIAL; - condTrue = gtNewHelperCallNode(helper, TYP_REF, 0, gtNewArgList(op2Var, gtClone(op1))); + condTrue = gtNewHelperCallNode(specialHelper, TYP_REF, 0, gtNewArgList(op2Var, gtClone(op1))); } else { @@ -9632,7 +9693,6 @@ void Compiler::impImportBlockCode(BasicBlock* block) IL_OFFSET nxtStmtOffs; GenTreePtr arrayNodeFrom, arrayNodeTo, arrayNodeToIndex; - bool expandInline; CorInfoHelpFunc helper; CorInfoIsAccessAllowedResult accessAllowedResult; CORINFO_HELPER_DESC calloutHelper; @@ -14109,7 +14169,8 @@ void Compiler::impImportBlockCode(BasicBlock* block) BOOL runtimeLookup; op2 = impTokenToHandle(&resolvedToken, &runtimeLookup); if (op2 == nullptr) - { // compDonotInline() + { + assert(compDonotInline()); return; } @@ -14127,6 +14188,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) tiRetVal = verMakeTypeInfo(resolvedToken.hClass); tiRetVal.NormaliseForStack(); } + JITDUMP("\n Importing UNBOX.ANY(refClass) as CASTCLASS\n"); op1 = impPopStack().val; goto CASTCLASS; } @@ -14156,19 +14218,13 @@ void Compiler::impImportBlockCode(BasicBlock* block) helper = info.compCompHnd->getUnBoxHelper(resolvedToken.hClass); assert(helper == CORINFO_HELP_UNBOX || helper == CORINFO_HELP_UNBOX_NULLABLE); - // We only want to expand inline the normal UNBOX helper; - expandInline = (helper == CORINFO_HELP_UNBOX); - - if (expandInline) - { - if (compCurBB->isRunRarely()) - { - expandInline = false; // not worth the code expansion - } - } + // Check legality and profitability of inline expansion for unboxing. + const bool canExpandInline = (helper == CORINFO_HELP_UNBOX); + const bool shouldExpandInline = !(compCurBB->isRunRarely() || opts.compDbgCode || opts.MinOpts()); - if (expandInline) + if (canExpandInline && shouldExpandInline) { + JITDUMP("\n Importing %s as inline sequence\n", opcode == CEE_UNBOX ? "UNBOX" : "UNBOX.ANY"); // we are doing normal unboxing // inline the common case of the unbox helper // UNBOX(exp) morphs into @@ -14212,6 +14268,8 @@ void Compiler::impImportBlockCode(BasicBlock* block) } else { + JITDUMP("\n Importing %s as helper call because %s\n", opcode == CEE_UNBOX ? "UNBOX" : "UNBOX.ANY", + canExpandInline ? "want smaller code or faster jitting" : "inline expansion not legal"); unsigned callFlags = (helper == CORINFO_HELP_UNBOX) ? 0 : GTF_EXCEPT; // Don't optimize, just call the helper and be done with it @@ -14373,6 +14431,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) // stack changes (in generic code a 'T' becomes a 'boxed T') if (!eeIsValueClass(resolvedToken.hClass)) { + JITDUMP("\n Importing BOX(refClass) as NOP\n"); verCurrentState.esStack[verCurrentState.esStackDepth - 1].seTypeInfo = tiRetVal; break; } @@ -14389,6 +14448,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) if (unboxResolvedToken.hClass == resolvedToken.hClass) { + JITDUMP("\n Importing BOX; UNBOX.ANY as NOP\n"); // Skip the next unbox.any instruction sz += sizeof(mdToken) + 1; break; diff --git a/src/jit/valuenum.cpp b/src/jit/valuenum.cpp index 0206173183a1..e4990d635a3c 100644 --- a/src/jit/valuenum.cpp +++ b/src/jit/valuenum.cpp @@ -7296,6 +7296,7 @@ void Compiler::fgValueNumberHelperCallFunc(GenTreeCall* call, VNFunc vnf, ValueN } break; + case VNF_Box: case VNF_BoxNullable: { // Generate unique VN so, VNForFunc generates a uniq value number for box nullable. @@ -7792,6 +7793,10 @@ VNFunc Compiler::fgValueNumberHelperMethVNFunc(CorInfoHelpFunc helpFunc) vnf = VNF_LoopCloneChoiceAddr; break; + case CORINFO_HELP_BOX: + vnf = VNF_Box; + break; + case CORINFO_HELP_BOX_NULLABLE: vnf = VNF_BoxNullable; break; diff --git a/src/jit/valuenumfuncs.h b/src/jit/valuenumfuncs.h index 2711b4f0569c..a1372182c8c6 100644 --- a/src/jit/valuenumfuncs.h +++ b/src/jit/valuenumfuncs.h @@ -126,6 +126,7 @@ ValueNumFuncDef(JitNew, 2, false, true, false) ValueNumFuncDef(JitNewArr, 3, false, true, false) ValueNumFuncDef(JitReadyToRunNew, 2, false, true, false) ValueNumFuncDef(JitReadyToRunNewArr, 3, false, true, false) +ValueNumFuncDef(Box, 3, false, false, false) ValueNumFuncDef(BoxNullable, 3, false, false, false) ValueNumFuncDef(LT_UN, 2, false, false, false) From aa70c0c4b98c167b4b347df79e1765d6727dac5a Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 9 Aug 2017 13:52:26 -0700 Subject: [PATCH 2/2] Keep box strategy as-is when optimizing. Fix diagnostic comment. --- src/jit/importer.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 4c00c5913e37..f0b5fb5ee8a0 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -5247,9 +5247,8 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) // structs is cheap. JITDUMP("\nCompiler::impImportAndPushBox -- handling BOX(value class) via"); bool canExpandInline = (boxHelper == CORINFO_HELP_BOX); - bool optForSize = !exprToBox->IsCall() && (operCls != nullptr) && - (opts.compDbgCode || opts.MinOpts() || compCurBB->isRunRarely()); - bool expandInline = canExpandInline && !optForSize; + bool optForSize = !exprToBox->IsCall() && (operCls != nullptr) && (opts.compDbgCode || opts.MinOpts()); + bool expandInline = canExpandInline && !optForSize; if (expandInline) { @@ -5373,11 +5372,10 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) } else { - JITDUMP(" helper call because: %s\n", !canExpandInline ? "optimizing for size" : "nullable"); + // Don't optimize, just call the helper and be done with it. + JITDUMP(" helper call because: %s\n", canExpandInline ? "optimizing for size" : "nullable"); assert(operCls != nullptr); - // Don't optimize, just call the helper and be done with it - // // Ensure that the value class is restored op2 = impTokenToHandle(pResolvedToken, nullptr, TRUE /* mustRestoreHandle */); if (op2 == nullptr)