For arm32, kill REG_PROFILER_RET_SCRATCH for LSRA but not for GC #40123
Conversation
Reworking of dotnet#37969. Block LSRA from using R2 around the profiler leave callback, but don't kill GC refs in R2, since late codegen will use R2 to temporarily hold return values around the callback. Fixes dotnet#37223.
Dotnet-GitSync-Bot
added
the
area-CodeGen-coreclr
|
@CarolEidt PTAL |
|
Added in the gcstress-extra tests. |
| #if defined(TARGET_ARM) | ||
| // profiler scratch remains gc live | ||
| result = RBM_PROFILER_LEAVE_TRASH & ~RBM_PROFILER_RET_SCRATCH; | ||
| #else |
There was a problem hiding this comment.
Can you please explain why this is only needed for TARGET_ARM and not other architectures?
There was a problem hiding this comment.
Arm is the only target that does this sort of return value sheltering in jitted code around the profiler leave hook.
No other architecture defines RBM_PROFILER_RET_SCRATCH.
|
Added libraries jitstress test. |
|
Failure rates in gcstress-extra and libraries-jitstress seem similar to recent numbers. All are gc stress crashes or (in one case) an incorrect result. No recurrence of #37223. |
| @@ -491,6 +491,8 @@ int LinearScan::BuildNode(GenTree* tree) | |||
|
|
|||
| case GT_RETURN: | |||
| srcCount = BuildReturn(tree); | |||
| killMask = getKillSetForReturn(); | |||
| BuildDefsWithKills(tree, 0, RBM_NONE, killMask); | |||
There was a problem hiding this comment.
I am OK with this fix as-is, but I think another option that might be slightly cleaner would be to replace the above 3 lines with:
buildInternalIntRegisterDefForNode(tree, RBM_PROFILER_ENTER_TRASH);
srcCount = BuildReturn(tree);
buildInternalRegisterUses();
Then target.h and emit.cpp could, I believe, remain unchanged. This basically says to the register allocator: "I need an extra register and it must be r2", which will cause it to ensure that r2 is free at the return. However, I haven't tested this approach. Note that normally I wouldn't do this separately (outside of an existing BuildXXX method), but in this case BuildReturn() is shared across platforms and already complex.
There was a problem hiding this comment.
Yeah, I mentioned trying something like that over in the issue but wasn't sure how to pull it off.
Seems like the cleanest solution would be to inject these calls early and handle them like any other call.
Not sure how to proceed -- if you're ok with this fix then approve? Else I can try the above.
There was a problem hiding this comment.
I'll approve; not sure it's worth iterating on.
…net#40123) Reworking of dotnet#37969. Block LSRA from using R2 around the profiler leave callback, but don't kill GC refs in R2, since late codegen will use R2 to temporarily hold return values around the callback. Fixes dotnet#37223. Co-authored-by: Carol Eidt <carol.eidt@microsoft.com>
ghost
locked as resolved and limited conversation to collaborators
Reworking of #37969. Block LSRA from using R2 around the profiler leave
callback, but don't kill GC refs in R2, since late codegen will use
R2 to temporarily hold return values around the callback.
Fixes #37223.