From 3126d5817036212f00856795a625d0c46aa83b45 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 19 Oct 2021 08:55:47 -0700 Subject: [PATCH] JIT: fix interaction of GDV and boxing (#60355) In #56126 we disabled GDV if the object being boxed came from a call with a hidden return buffer pointer. Fix this and enable GDV for these cases by inserting the box allocation statements before the call. --- src/coreclr/jit/gentree.cpp | 6 +- src/coreclr/jit/importer.cpp | 118 +++++++++++++++++++++++++---------- 2 files changed, 91 insertions(+), 33 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 6b573c3d4adf0..5329c462d39f6 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -7428,7 +7428,11 @@ GenTree* Compiler::gtNewBitCastNode(var_types type, GenTree* arg) // Return Value: // Returns GT_ALLOCOBJ node that will be later morphed into an // allocation helper call or local variable allocation on the stack. - +// +// Node creation can fail for inlinees when the type described by pResolvedToken +// can't be represented in jitted code. If this happens, this method will return +// nullptr. +// GenTreeAllocObj* Compiler::gtNewAllocObjNode(CORINFO_RESOLVED_TOKEN* pResolvedToken, bool useParent) { const bool mustRestoreHandle = true; diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 22cd404f4d333..82f3219d6d04f 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -6865,59 +6865,113 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) // the opcode stack becomes empty impBoxTempInUse = true; + // Remember the current last statement in case we need to move + // a range of statements to ensure the box temp is initialized + // before it's used. + // + Statement* const cursor = impLastStmt; + const bool useParent = false; op1 = gtNewAllocObjNode(pResolvedToken, useParent); if (op1 == nullptr) { + // If we fail to create the newobj node, we must be inlining + // and have run across a type we can't describe. + // + assert(compDonotInline()); return; } - /* Remember that this basic block contains 'new' of an object, and so does this method */ + // Remember that this basic block contains 'new' of an object, + // and so does this method + // compCurBB->bbFlags |= BBF_HAS_NEWOBJ; optMethodFlags |= OMF_HAS_NEWOBJ; - GenTree* asg = gtNewTempAssign(impBoxTemp, op1); - + // Assign the boxed object to the box temp. + // + GenTree* asg = gtNewTempAssign(impBoxTemp, op1); Statement* asgStmt = impAppendTree(asg, (unsigned)CHECK_SPILL_NONE, impCurStmtOffs); - op1 = gtNewLclvNode(impBoxTemp, TYP_REF); - op2 = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); - op1 = gtNewOperNode(GT_ADD, TYP_BYREF, op1, op2); - - if (varTypeIsStruct(exprToBox)) + // If the exprToBox is a call that returns its value via a ret buf arg, + // move the assignment statement(s) before the call (which must be a top level tree). + // + // We do this because impAssignStructPtr (invoked below) will + // back-substitute into a call when it sees a GT_RET_EXPR and the call + // has a hidden buffer pointer, So we need to reorder things to avoid + // creating out-of-sequence IR. + // + if (varTypeIsStruct(exprToBox) && exprToBox->OperIs(GT_RET_EXPR)) { - // Workaround for GitHub issue 53549. - // - // If the struct being boxed is returned via hidden buffer and comes from an inline/gdv candidate, - // the IR we produce after importation is out of order: - // - // call (&(box-temp + 8), ....) - // box-temp = newobj - // ret-val from call (void) - // ... box-temp (on stack) - // - // For inline candidates this bad ordering gets fixed up during inlining, but for GDV candidates - // the GDV expansion is such that the newobj follows the call as in the above. - // - // This is nontrivial to fix in GDV, so in these (rare) cases we simply disable GDV. - // - if (exprToBox->OperIs(GT_RET_EXPR)) + GenTreeCall* const call = exprToBox->AsRetExpr()->gtInlineCandidate->AsCall(); + + if (call->HasRetBufArg()) { - GenTreeCall* const call = exprToBox->AsRetExpr()->gtInlineCandidate->AsCall(); + JITDUMP("Must insert newobj stmts for box before call [%06u]\n", dspTreeID(call)); + + // Walk back through the statements in this block, looking for the one + // that has this call as the root node. + // + // Because gtNewTempAssign (above) may have added statements that + // feed into the actual assignment we need to move this set of added + // statements as a group. + // + // Note boxed allocations are side-effect free (no com or finalizer) so + // our only worries here are (correctness) not overlapping the box temp + // lifetime and (perf) stretching the temp lifetime across the inlinee + // body. + // + // Since this is an inline candidate, we must be optimizing, and so we have + // a unique box temp per call. So no worries about overlap. + // + assert(!opts.OptimizationDisabled()); - if (call->IsGuardedDevirtualizationCandidate() && call->HasRetBufArg()) + // Lifetime stretching could addressed with some extra cleverness--sinking + // the allocation back down to just before the copy, once we figure out + // where the copy is. We defer for now. + // + Statement* insertBeforeStmt = cursor; + noway_assert(insertBeforeStmt != nullptr); + + while (true) { - JITDUMP("Disabling GDV for [%06u] because of in-box struct return\n"); - call->ClearGuardedDevirtualizationCandidate(); - if (call->IsVirtualStub()) + if (insertBeforeStmt->GetRootNode() == call) { - JITDUMP("Restoring stub addr %p from guarded devirt candidate info\n", - dspPtr(call->gtGuardedDevirtualizationCandidateInfo->stubAddr)); - call->gtStubCallStubAddr = call->gtGuardedDevirtualizationCandidateInfo->stubAddr; + break; } + + // If we've searched all the statements in the block and failed to + // find the call, then something's wrong. + // + noway_assert(insertBeforeStmt != impStmtList); + + insertBeforeStmt = insertBeforeStmt->GetPrevStmt(); } + + // Found the call. Move the statements comprising the assignment. + // + JITDUMP("Moving " FMT_STMT "..." FMT_STMT " before " FMT_STMT "\n", cursor->GetNextStmt()->GetID(), + asgStmt->GetID(), insertBeforeStmt->GetID()); + assert(asgStmt == impLastStmt); + do + { + Statement* movingStmt = impExtractLastStmt(); + impInsertStmtBefore(movingStmt, insertBeforeStmt); + insertBeforeStmt = movingStmt; + } while (impLastStmt != cursor); } + } + // Create a pointer to the box payload in op1. + // + op1 = gtNewLclvNode(impBoxTemp, TYP_REF); + op2 = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + op1 = gtNewOperNode(GT_ADD, TYP_BYREF, op1, op2); + + // Copy from the exprToBox to the box payload. + // + if (varTypeIsStruct(exprToBox)) + { assert(info.compCompHnd->getClassSize(pResolvedToken->hClass) == info.compCompHnd->getClassSize(operCls)); op1 = impAssignStructPtr(op1, exprToBox, operCls, (unsigned)CHECK_SPILL_ALL); }