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: Keep delegate object alive during invoke #105099

Draft
wants to merge 34 commits into
base: main
Choose a base branch
from
Draft

JIT: Keep delegate object alive during invoke #105099

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
4a90d51
Disable Comparer_get_Default test on win-arm64-crossgen
EgorBo Jul 18, 2024
76c6f4d
Keep 'this' alive for delegate invoke
EgorBo Jul 18, 2024
49b10a6
Update issues.targets
EgorBo Jul 18, 2024
8bf8365
Update lower.cpp
EgorBo Jul 18, 2024
96543b3
less conservative version
EgorBo Jul 18, 2024
5db07f8
fix define
EgorBo Jul 18, 2024
118c8ad
Apply Jakob's suggestion
EgorBo Jul 18, 2024
ffe5317
GT_STOP_NOGC
EgorBo Jul 19, 2024
5558203
Merge branch 'main' into keep-this-alive-fptr
EgorBo Jul 19, 2024
bfc872f
Update lower.cpp
EgorBo Jul 19, 2024
300ab49
Address feedback
EgorBo Jul 19, 2024
26da6ab
Address feedback
EgorBo Jul 19, 2024
f95c267
Merge branch 'main' of https://github.com/dotnet/runtime into keep-th…
EgorBo Aug 1, 2024
b35c892
handle tail calls
EgorBo Aug 1, 2024
255c5bd
Merge branch 'main' into keep-this-alive-fptr
EgorBo Aug 1, 2024
ffdc8e5
remove bogus assert
EgorBo Aug 1, 2024
9e34a9a
Merge branch 'keep-this-alive-fptr' of github.com:EgorBo/runtime-1 in…
EgorBo Aug 1, 2024
1dac215
Update lower.cpp
EgorBo Aug 1, 2024
7cb1d65
Update lower.cpp
EgorBo Aug 1, 2024
f0964fd
Update lower.cpp
EgorBo Aug 1, 2024
126fca9
Update lower.cpp
EgorBo Aug 2, 2024
c28b235
Merge branch 'main' of github.com:dotnet/runtime into keep-this-alive…
EgorBo Oct 10, 2024
65d7909
test
EgorBo Oct 10, 2024
dded5c9
test2
EgorBo Oct 10, 2024
dca303b
test
EgorBo Oct 10, 2024
95b6301
Clean up
EgorBo Oct 10, 2024
2377322
Update lower.cpp
EgorBo Oct 10, 2024
bef38f3
clean up
EgorBo Oct 10, 2024
9fba5a3
Update lower.cpp
EgorBo Oct 10, 2024
9ccc937
Update lower.cpp
EgorBo Oct 11, 2024
2ab1e61
Merge branch 'main' of github.com:dotnet/runtime into keep-this-alive…
EgorBo Oct 11, 2024
ef49ee6
test
EgorBo Oct 11, 2024
f4061ea
Merge branch 'main' into keep-this-alive-fptr
EgorBo Nov 4, 2024
a5c4b81
Merge branch 'main' into keep-this-alive-fptr
EgorBo Nov 19, 2024
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
4 changes: 4 additions & 0 deletions src/coreclr/jit/codegenarmarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,10 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
GetEmitter()->emitDisableGC();
break;

case GT_STOP_NONGC:
GetEmitter()->emitEnableGC();
break;

case GT_START_PREEMPTGC:
// Kill callee saves GC registers, and create a label
// so that information gets propagated to the emitter.
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/codegenloongarch64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4206,6 +4206,10 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
GetEmitter()->emitDisableGC();
break;

case GT_STOP_NONGC:
GetEmitter()->emitEnableGC();
break;

case GT_START_PREEMPTGC:
// Kill callee saves GC registers, and create a label
// so that information gets propagated to the emitter.
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/codegenriscv64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4285,6 +4285,10 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
GetEmitter()->emitDisableGC();
break;

case GT_STOP_NONGC:
GetEmitter()->emitEnableGC();
break;

case GT_START_PREEMPTGC:
// Kill callee saves GC registers, and create a label
// so that information gets propagated to the emitter.
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/codegenxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1873,6 +1873,10 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
case GT_START_NONGC:
GetEmitter()->emitDisableGC();
break;

case GT_STOP_NONGC:
GetEmitter()->emitEnableGC();
break;
#endif // !defined(JIT32_GCENCODER)

case GT_START_PREEMPTGC:
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11708,6 +11708,7 @@ class GenTreeVisitor
case GT_SETCC:
case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_START_PREEMPTGC:
case GT_PROF_HOOK:
#if defined(FEATURE_EH_WINDOWS_X86)
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4365,6 +4365,7 @@ void GenTree::VisitOperands(TVisitor visitor)
case GT_SETCC:
case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_START_PREEMPTGC:
case GT_PROF_HOOK:
#if defined(FEATURE_EH_WINDOWS_X86)
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6475,6 +6475,7 @@ bool GenTree::TryGetUse(GenTree* operand, GenTree*** pUse)
case GT_SETCC:
case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_START_PREEMPTGC:
case GT_PROF_HOOK:
#if defined(FEATURE_EH_WINDOWS_X86)
Expand Down Expand Up @@ -10011,6 +10012,7 @@ GenTreeUseEdgeIterator::GenTreeUseEdgeIterator(GenTree* node)
case GT_SETCC:
case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_START_PREEMPTGC:
case GT_PROF_HOOK:
#if defined(FEATURE_EH_WINDOWS_X86)
Expand Down Expand Up @@ -12158,6 +12160,7 @@ void Compiler::gtDispLeaf(GenTree* tree, IndentStack* indentStack)
case GT_NOP:
case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_START_PREEMPTGC:
case GT_PROF_HOOK:
case GT_CATCH_ARG:
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/gtlist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ GTNODE(SWITCH , GenTreeOp ,0,1,GTK_UNOP|GTK_NOVALUE)
GTNODE(NO_OP , GenTree ,0,0,GTK_LEAF|GTK_NOVALUE) // A NOP that cannot be deleted.

GTNODE(START_NONGC , GenTree ,0,0,GTK_LEAF|GTK_NOVALUE|DBK_NOTHIR) // Starts a new instruction group that will be non-gc interruptible.
GTNODE(STOP_NONGC , GenTree ,0,0,GTK_LEAF|GTK_NOVALUE|DBK_NOTHIR) // Tries to end the non-gc interruptible instruction group
GTNODE(START_PREEMPTGC , GenTree ,0,0,GTK_LEAF|GTK_NOVALUE|DBK_NOTHIR) // Starts a new instruction group where preemptive GC is enabled.
GTNODE(PROF_HOOK , GenTree ,0,0,GTK_LEAF|GTK_NOVALUE|DBK_NOTHIR) // Profiler Enter/Leave/TailCall hook.

Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/liveness.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1438,6 +1438,7 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
case GT_SWITCH:
case GT_RETFILT:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_START_PREEMPTGC:
case GT_PROF_HOOK:
#if defined(FEATURE_EH_WINDOWS_X86)
Expand Down
28 changes: 26 additions & 2 deletions src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2691,6 +2691,7 @@ GenTree* Lowering::LowerCall(GenTree* node)
// note that everything generated from this point might run AFTER the outgoing args are placed
GenTree* controlExpr = nullptr;
bool callWasExpandedEarly = false;
bool endNogcBeforeCall = false;

// for x86, this is where we record ESP for checking later to make sure stack is balanced

Expand All @@ -2699,7 +2700,7 @@ GenTree* Lowering::LowerCall(GenTree* node)
// an indirect call.
if (call->IsDelegateInvoke())
{
controlExpr = LowerDelegateInvoke(call);
controlExpr = LowerDelegateInvoke(call, &endNogcBeforeCall);
}
else
{
Expand Down Expand Up @@ -2807,6 +2808,13 @@ GenTree* Lowering::LowerCall(GenTree* node)
}

ContainCheckCallOperands(call);

if (endNogcBeforeCall)
{
GenTree* stopNonGCNode = new (comp, GT_STOP_NONGC) GenTree(GT_STOP_NONGC, TYP_VOID);
BlockRange().InsertBefore(call, stopNonGCNode);
}

JITDUMP("lowering call (after):\n");
DISPTREERANGE(BlockRange(), call);
JITDUMP("\n");
Expand Down Expand Up @@ -5491,8 +5499,10 @@ GenTree* Lowering::LowerDirectCall(GenTreeCall* call)
return result;
}

GenTree* Lowering::LowerDelegateInvoke(GenTreeCall* call)
GenTree* Lowering::LowerDelegateInvoke(GenTreeCall* call, bool* shouldEndNogcBeforeCall)
{
assert(shouldEndNogcBeforeCall != nullptr);
*shouldEndNogcBeforeCall = false;
noway_assert(call->gtCallType == CT_USER_FUNC);

assert((comp->info.compCompHnd->getMethodAttribs(call->gtCallMethHnd) &
Expand Down Expand Up @@ -5551,6 +5561,20 @@ GenTree* Lowering::LowerDelegateInvoke(GenTreeCall* call)
BlockRange().Remove(thisArgNode);
BlockRange().InsertBefore(call, newThisAddr, newThis, thisArgNode);

#if !defined(TARGET_XARCH)
if (comp->GetInterruptible())
{
// If the target's backend doesn't support indirect calls with immediate operands (contained)
// and the method is marked as interruptible, we need to insert a GT_START_NONGC before the call.
// to keep the delegate object alive while we're obtaining the function pointer.
GenTree* startNonGCNode = new (comp, GT_START_NONGC) GenTree(GT_START_NONGC, TYP_VOID);
BlockRange().InsertAfter(thisArgNode, startNonGCNode);

// We should try to end the non-GC region just before the actual call.
*shouldEndNogcBeforeCall = true;
}
#endif

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be cleaner to insert both START and STOP at the end of LowerCall instead of doing half in here and half in LowerCall.

Also, is inserting the stop node before the call node really correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is inserting the stop node before the call node really correct?

Why not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GC is illegal when we are on the blr instruction -- it is not illegal when we are on the ldr instruction. https://www.diffchecker.com/NwHIxdIA/ looks like it makes the GC illegal on the ldr instruction but not on the blr instruction.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can GC happen on call instructions before we entered the call?
If I need to extend the nogc past the call then it's easier for my PR, I just though that I should not add a call to nogc block (because GC may happens inside the call)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can GC happen on call instructions before we entered the call?

Yes, I think that's the problem here. If GC happens on that call instruction nothing indicates the delegate is live and thus the target we are going to jump to may be collected.

I just though that I should not add a call to nogc block (because GC may happens inside the call)

Hmm, I'm not totally sure what happens, but the safepoint should logically be on the return address, not on the call, so I think we should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just wondering if we have any debug mechanism that checks that all calls inside nogc blocks can't trigger GC even internally, but I presume we don't otherwise we'd catch the recent bug with BULKBARRIER earlier

ContainCheckIndir(newThis->AsIndir());

// the control target is
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/lower.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ class Lowering final : public Phase
#if !defined(WINDOWS_AMD64_ABI)
GenTreeLclVar* SpillStructCallResult(GenTreeCall* call) const;
#endif // WINDOWS_AMD64_ABI
GenTree* LowerDelegateInvoke(GenTreeCall* call);
GenTree* LowerDelegateInvoke(GenTreeCall* call, bool* shouldStopNongcBeforeCall);
GenTree* LowerIndirectNonvirtCall(GenTreeCall* call);
GenTree* LowerDirectCall(GenTreeCall* call);
GenTree* LowerNonvirtPinvokeCall(GenTreeCall* call);
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/lsraarm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,7 @@ int LinearScan::BuildNode(GenTree* tree)

case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_PROF_HOOK:
srcCount = 0;
assert(dstCount == 0);
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/lsraarm64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,7 @@ int LinearScan::BuildNode(GenTree* tree)

case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
srcCount = 0;
assert(dstCount == 0);
break;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/lsraloongarch64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ int LinearScan::BuildNode(GenTree* tree)

case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
srcCount = 0;
assert(dstCount == 0);
break;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/lsrariscv64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ int LinearScan::BuildNode(GenTree* tree)

case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
srcCount = 0;
assert(dstCount == 0);
break;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/lsraxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ int LinearScan::BuildNode(GenTree* tree)

case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
srcCount = 0;
assert(dstCount == 0);
break;
Expand Down
Loading