JIT: update tail call IR validity checks#94130
Conversation
Adapt `fgValidateIRForTailCall` to use as a utility to verify that the JIT has not added IR that would make a tail call invalid. Currently all our cases pass this check so no tail calls are invalidated. Remove `fgCheckStmtAfterTailCall` as it did less thorough and less correct checking. Contributes to dotnet#93246.
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsAdapt Remove Contributes to #93246.
|
|
FYI @dotnet/jit-contrib No diffs expected. See #93997 (comment) for context. Once we move return merging earlier, we may end up with a small number of cases where phases between |
| for (Statement* stmt = compCurStmt; stmt != nullptr; stmt = stmt->GetNextStmt()) | ||
| for (Statement* stmt = compCurStmt; visitor.IsValid() && (stmt != nullptr); stmt = stmt->GetNextStmt()) | ||
| { | ||
| visitor.WalkTree(stmt->GetRootNodePointer(), nullptr); |
There was a problem hiding this comment.
Nit: I think it would be a bit cleaner to return WALK_ABORT on validation failure in the visitor (looks like at least GT_RETURN needs to be fixed) and have early exits for when WalkTree returns that.
|
Hmm, I just realized this is problematic for explicit tailcalls... We cannot be allowed to invalidate those. Is it possible to have #93997 avoid merging (explicit) tailcall cases instead? |
Yes, we can do something like that. I am currently thinking:
I can adapt this PR to go back to assertion checking like it was doing before, and drop |
|
That sounds good to me. |
AndyAyersMS
changed the title
|
Revised per above. |
|
Failures are known. |
|
/azp run runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2 participants
Remove
fgCheckStmtAfterTailCallas it did less thorough and less correct checking. We will rely onfgValidateIRForTailCallto sanity check that there is no unexpcted IR after a tail call.Contributes to #93246.