Skip to content

JIT: Keep delegate object alive during invoke #105099

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

Closed
wants to merge 34 commits into from
Closed

JIT: Keep delegate object alive during invoke #105099

Changes from 6 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
11 changes: 11 additions & 0 deletions src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5551,6 +5551,17 @@ GenTree* Lowering::LowerDelegateInvoke(GenTreeCall* call)
BlockRange().Remove(thisArgNode);
BlockRange().InsertBefore(call, newThisAddr, newThis, thisArgNode);

#if !defined(TARGET_XARCH)
// Keep delegate alive if the target can't do an indirect call with an immediate operand
// and only in fully interruptible code.
if (comp->GetInterruptible())
{
GenTree* baseClone = comp->gtCloneExpr(base);
GenTree* keepBaseAlive = comp->gtNewKeepAliveNode(baseClone);
BlockRange().InsertBefore(call, baseClone, keepBaseAlive);
Copy link
Member

@jakobbotsch jakobbotsch Jul 18, 2024

Choose a reason for hiding this comment

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

This InsertBefore only works due to implementation details of the backend (lazy liveness updates for GC information about registers), which is quite subtle. But of course the fully ideal solution requires adding a new operand to GenTreeCall and special casing it throughout the backend as an invisible use, which is probably a bit overkill.

I wonder if we in the uncontained case wouldn't be better off just by inserting GT_START_NONGC and creating a nogc region on the single call instruction. Given that we already have a call and hence a GC safe point this does not seem like it would be very harmful; in a less conservative JIT this basic block might have simply always been partially interruptible for that reason.

Copy link
Member Author

@EgorBo EgorBo Jul 18, 2024

Choose a reason for hiding this comment

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

hm.. interesting idea! applied

Copy link
Member Author

@EgorBo EgorBo Jul 18, 2024

Choose a reason for hiding this comment

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

The new diff for the snippet in the issue: https://www.diffchecker.com/CiuxVSBL/

Copy link
Member

@jakobbotsch jakobbotsch Jul 18, 2024

Choose a reason for hiding this comment

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

I think you will also need to add a STOP_NOGC to turn it off again. Looks like we don't have that today.

Copy link
Member Author

@EgorBo EgorBo Jul 18, 2024

Choose a reason for hiding this comment

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

I think you will also need to add a STOP_NOGC to turn it off again. Looks like we don't have that today.

I guess it's not from a correctness stand point, just to reduce the nongc area if we have a lot more statements in the current block?

Copy link
Member Author

@EgorBo EgorBo Jul 18, 2024

Choose a reason for hiding this comment

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

I guess this one is tricky - we might end whatever was expected to be nongc even before we emitted the start_nogc

Copy link
Member

@jakobbotsch jakobbotsch Jul 18, 2024

Choose a reason for hiding this comment

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

It's counted, so it doesn't turn off nogc until the count gets to zero. But regardless I wouldn't expect there to ever be another active nogc region in this case

Copy link
Member

@jakobbotsch jakobbotsch Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess it's not from a correctness stand point, just to reduce the nongc area if we have a lot more statements in the current block?

I'm not really sure how the VM behaves if the return address is in a nogc region – but in any case it's unnecessary. I think we only need it to be active from the load of the target address (which is the last use) to the call – should be just one instruction in the end

}
#endif

ContainCheckIndir(newThis->AsIndir());

// the control target is
Expand Down