JIT: Improve modelling of AsyncResumedDef store in value numbering - #132423
Conversation
VN previously gave all call-produced local definitions a fresh unique value number, including the async "resumed" indicator definition. In two cases we know the value is 1 after the call: 1. The call always suspends, so the code after it is only reachable via resumption. 2. The indicator is already known to be 1 coming into the call; it is only ever stored 1 (on resumption), so it stays 1. Model both, which lets the resumed uses at subsequent async calls fold to a constant. Fixes dotnet#131576 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a003aac4-5931-47f0-87fc-26586671fb3c
github-actions
Bot
added
the
area-CodeGen-coreclr
There was a problem hiding this comment.
Pull request overview
This PR updates RyuJIT value numbering for async calls so the “resumed” indicator local definition is modeled as a known constant 1 in cases where it is provably 1 after the call (e.g., always-suspending awaits or when the indicator is already known to be 1).
Changes:
- Adds logic in
fgValueNumberCallto detect when the async resumed indicator must be1after an async call. - Uses that knowledge to assign a constant VN to the async resumed local definition instead of a fresh unknown VN.
The store value was derived from the liberal VN of the resumed use and then applied to both kinds. Derive each kind from the corresponding kind of the use instead. For an always-suspending call the value is 1 for both kinds. Any kind whose value is not known falls back to a new unique VN, using the same one for both when neither is known. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a003aac4-5931-47f0-87fc-26586671fb3c
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🔵 Needs a closer look
The changes affect core JIT value numbering and async transformation semantics, and warrant careful human validation (including behavior/codegen impact) beyond what I can conclusively verify here.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS Diffs. But also an optimization that may kick in more often as we start improving/implementing other optimization, e.g. inlining can reveal these cases. We could go a bit further here. Currently the async transformation computes for each basic block (and by extension, each async call) whether we can get there having already suspended. It is used to optimize out the "can we reuse a continuation" check. We could replace this with a check for whether the indicator is known to be true at the point of the call. I may look into that clean up separately, but this analysis is not that expensive regardless since it only runs on a per-block level. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 346b7c7c-5ce9-4029-ae65-a6240efee652
There was a problem hiding this comment.
🔵 Needs a closer look
It changes CoreCLR JIT value numbering and async suspension transformation logic, which is high-blast-radius and merits final human review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/coreclr/jit/async.cpp:3619
- When an enclosing frame's resumed indicator is proven constant non-zero, the debug message says we're skipping the rest of the tail, but the code uses
continue, so later iterations still run the member-registration assertions and loop bookkeeping. Usingbreakbetter matches the intent and avoids unnecessary work (and potential asserts) once the remainder is known to be a no-op.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core JIT value numbering and async transformation codegen behavior, so it warrants careful human review for correctness and unintended downstream optimization effects.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/azp run runtime |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines: Successfully started running 2 pipeline(s). |
|
/ba-g Failure is #133418 and actually known, reported it in first responders |
VN previously gave all call-produced local definitions a fresh unique value number, including the async "resumed" indicator definition. In two cases we know the value is 1 after the call:
Model both, which lets the resumed uses at subsequent async calls fold to a constant. In addition add an optimization to the async transformation that avoids emitting call to functions that are known no-ops when the resumed indicator is true.
Fix #131576