JIT: don't stress-tailcall named intrinsics (#122479)#128336
JIT: don't stress-tailcall named intrinsics (#122479)#128336AndyAyersMS wants to merge 1 commit into
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @VSadov |
There was a problem hiding this comment.
Pull request overview
Updates CoreCLR JIT tailcall-stress handling to avoid treating intrinsic calls as explicit tailcalls, and removes the corresponding temporary runtime workaround.
Changes:
- JIT importer: gate tailcall-stress “explicit tailcall” promotion on
CORINFO_FLG_INTRINSIC. - System.Private.CoreLib: remove the temporary
[MethodImpl(NoInlining | NoOptimization)]workaround onThread.ClearWaitSleepJoinState().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/jit/importer.cpp | Skips stress tailcall promotion for intrinsic callees to avoid later-phase inconsistencies/asserts. |
| src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs | Removes prior workaround attributes/comment from ClearWaitSleepJoinState. |
Under STRESS_TAILCALL the importer marks call+ret patterns with PREFIX_TAILCALL_EXPLICIT (and may later add PREFIX_TAILCALL_IMPLICIT) before it knows whether the callee is a named intrinsic. For intrinsics that are imported as non-CALL IR (e.g. GC.KeepAlive -> GT_KEEPALIVE), this leaves a BBJ_RETURN block whose last statement is neither a GT_RETURN nor a GT_CALL, which trips an assert in fgInsertStmtNearEnd when `Insert GC Polls` later runs on methods that contain [SuppressGCTransition] calls (e.g. Thread.ClearWaitSleepJoinState). Fix by treating intrinsic callees as having failed stress-mode validation: set passedStressModeValidation = false so neither the explicit nor the subsequent implicit tailcall promotion is applied to them. Removes the temporary workaround in Thread.CoreCLR.cs added by dotnet#122652, and adds a small JitBlue regression test that exercises the Thread.Sleep -> Thread.ClearWaitSleepJoinState path so the jitstress legs will catch any future regression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
AndyAyersMS
force-pushed
the
fix-122479-tailcall-stress-intrinsic
branch
from
fcd8716 to
429afcc
Compare
|
Fixes #122479 |
|
@jakobbotsch PTAL |
| // non-CALL IR nodes (e.g. GC.KeepAlive -> GT_KEEPALIVE), which would | ||
| // leave a BBJ_RETURN block that doesn't end in a CALL/RETURN and | ||
| // confuse later phases (see | ||
| // https://github.com/dotnet/runtime/issues/122479). Suppress both the |
There was a problem hiding this comment.
This also matches hardware and JIT intrinsics, correct?
I know its not exactly related since this is covering stress mode, but we have some other "pessimizations" around intrinsics and tail calls such as https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/importercalls.cpp#L10212-L10225 where we may refuse to import it as non-CALL IR.
I wonder if for cases that are known to expand to trivial IR, like KEEPALIVE or most HWIntrinsics, if its worth just blanket avoiding tail calls altogether or if we need to centrally handle tail. call intrinsic to ensure we never try to expand them (if its required to respect the tail call)
There was a problem hiding this comment.
I recall we have some vague notion of centralizing the tail call logic somewhere, though I can't remember now if it is to move it all earlier (maybe as a post-importer phase) or move it all later, into morph. Either way we'd have resolved intrinsics into closer-to-final IR before making decisions about tail calls.
Under STRESS_TAILCALL the importer marks call+ret patterns with PREFIX_TAILCALL_EXPLICIT before it knows whether the callee is a named intrinsic. For intrinsics that are imported as non-CALL IR (e.g. GC.KeepAlive -> GT_KEEPALIVE), this leaves a BBJ_RETURN block whose last statement is neither a GT_RETURN nor a GT_CALL, which trips an assert in fgInsertStmtNearEnd when
Insert GC Pollslater runs on methods that contain [SuppressGCTransition] calls (e.g. Thread.ClearWaitSleepJoinState).Fix by skipping stress-tailcall promotion when the callee has CORINFO_FLG_INTRINSIC set.
Removes the temporary workaround in Thread.CoreCLR.cs added by #122652.
Fixes #122479