For arm32, kill REG_PROFILER_RET_SCRATCH for LSRA but not for GC#40123
For arm32, kill REG_PROFILER_RET_SCRATCH for LSRA but not for GC#40123AndyAyersMS merged 1 commit intodotnet:masterfrom
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. |
| 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.
CarolEidt
left a comment
CarolEidt
left a comment
There was a problem hiding this comment.
Thanks for all your work on this!
…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 <[email protected]>
ghost
locked as resolved and limited conversation to collaborators
5 participants
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.