Conversation
There was a problem hiding this comment.
Pull request overview
Addresses a CoreCLR JIT debug assertion during tailcall validation when inlining introduces small-type-normalizing casts (e.g., bool normalization) between a tailcall and the return, which was triggering failures in tvos-arm64 Debug crossgen2/R2R scenarios (issue #126457).
Changes:
- Update
fgValidateIRForTailCall(DEBUG-only validation) to accept/skip normalizingGT_CASTnodes when tracing a tailcall’s return value to the finalGT_RETURN. - Add a new JIT regression IL test (
Runtime_126457) to exercise the inlinee+tailcall+separate-return-block pattern implicated in the crash.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/jit/morph.cpp | Extends tailcall IR validation to tolerate small-type normalizing casts introduced by the inliner. |
| src/tests/JIT/Regression/JitBlue/Runtime_126457/Runtime_126457.ilproj | Adds a new IL-based JIT regression test project configuration. |
| src/tests/JIT/Regression/JitBlue/Runtime_126457/Runtime_126457.il | Adds IL repro that drives inlining + tailcall + return-flow shape relevant to the bug. |
| // (int -> ubyte -> int for bool-return-calls, for example) | ||
| // In the jit the callee is responsible for normalizing, so a tailcall | ||
| // can freely bypass this extra cast | ||
| assert(IsNormalizingCast(tree) && |
There was a problem hiding this comment.
This has to be the right type of normalizing cast -- e.g. if the inserted cast is zero-extending, then the call should also have a zero-extended return of the right size. I think you can just replace this with tree->AsCast()->CastToType() == m_tailcall->gtReturnType.
There was a problem hiding this comment.
Since we have fgCastNeeded you can also just use that here: fgCastNeeded(m_tailcall, tree->CastToType()))
| { | ||
| if (node->OperIs(GT_CAST) && !m_compiler->fgCastNeeded(m_tailcall, node->AsCast()->CastToType())) | ||
| { | ||
| node = node->AsCast()->CastOp(); |
There was a problem hiding this comment.
Why move this here? It looks inconsistent with how the other cases are handled
There was a problem hiding this comment.
Are you talking about this cast stripping logic? We need this here because the GT_RETURN node calls into ValidateUse
or are you referring to the fgCastNeeded check? I can move that back into the assert, thats probably better
| .assembly extern System.Runtime | ||
| { | ||
| .publickeytoken = (B0 3F 5F 7F 11 D5 0A 3A) | ||
| } | ||
|
|
||
| .assembly Runtime_126457 { } | ||
| .module Runtime_126457.dll | ||
|
|
There was a problem hiding this comment.
New IL source file is missing the standard .NET Foundation/MIT license header comment that most JIT regression .il sources include. Please add the license header at the top of this file to match repo conventions.
| pop | ||
| ldc.i4.0 | ||
| br.s DONE | ||
| CALL: |
There was a problem hiding this comment.
This regression is specifically about tailcall validation; to make the test deterministic across JIT configs, consider forcing the call to be a tailcall by adding the tail. IL prefix before the callvirt to ISite::tailcall() (many existing tailcall IL regressions do this). Without it, the JIT may decide not to tailcall-optimize and the test may not cover the intended scenario.
| CALL: | |
| CALL: | |
| tail. |
For a bool-typed inlinee that contains a tail call, where the return is in a seperate block from the tailcall (for a single-return method, for example), the inliner inserts an int->ubyte->int normalizing cast
The tail-call validation in morph doesn't expect a cast there. However, a normalizing cast here may be safely ignored for tailcalls because
1 - If the calling function was not inlined, the normalizing cast is intead inserted later in morph after the tailcall transformation, and
2 - Callees are responsible for this adjustment anyway so the tailcall return value would have already been normalized
fixes #126457