Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JIT: fix interaction of GDV and boxing #60355

Merged
merged 2 commits into from
Oct 19, 2021
Merged

JIT: fix interaction of GDV and boxing #60355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8326ca
JIT: fix interaction of GDV and boxing
AndyAyersMS Oct 13, 2021
24f65b1
revise
AndyAyersMS Oct 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
118 changes: 86 additions & 32 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down