JIT: Preparatory refactoring to support multiple defs per single node #117662
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Replace `GenTree::DefinesLocal` by a `GenTree::VisitLocalDefs` to prepare for being able to allow a single node to have multiple definitions. Update uses to use the new function.
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
/azp run runtime-coreclr superpmi-diffs |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS No diffs are expected, but the diffs run didn't succeed because of a JIT-EE GUID change. Kicked it off again. |
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors how the JIT handles local variable definitions by replacing the single-definition method GenTree::DefinesLocal with a visitor pattern method GenTree::VisitLocalDefs. This change prepares the JIT infrastructure to support multiple definitions per node, which will be needed for async call optimizations where calls need to define both the return value and a suspension state indicator.
Key changes include:
- Introduces a new
LocalDefstruct to encapsulate definition metadata (local, entirety, offset, size) - Replaces
DefinesLocalcalls withVisitLocalDefsthroughout the codebase - Updates SSA builder, value numbering, liveness analysis, and other passes to use the visitor pattern
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/gentree.h | Defines LocalDef struct and VisitLocalDefs interface |
| src/coreclr/jit/compiler.hpp | Implements VisitLocalDefs template method and HasAnyLocalDefs helper |
| src/coreclr/jit/gentree.cpp | Removes old DefinesLocal method implementation |
| src/coreclr/jit/valuenum.cpp | Updates value numbering to use visitor pattern for call definitions |
| src/coreclr/jit/ssabuilder.cpp | Refactors SSA renaming to handle multiple definitions via visitor |
| src/coreclr/jit/liveness.cpp | Updates liveness analysis to use visitor pattern for call definitions |
| src/coreclr/jit/morph.cpp | Updates variable definition flagging to use visitor pattern |
| src/coreclr/jit/copyprop.cpp | Updates copy propagation to use visitor pattern for definitions |
| src/coreclr/jit/treelifeupdater.cpp | Updates tree life updater to use visitor for call definitions |
| src/coreclr/jit/lower.cpp | Updates lowering to use visitor for tracking stored locals |
| src/coreclr/jit/flowgraph.cpp | Updates flow graph analysis to use visitor pattern |
| src/coreclr/jit/fgdiagnostic.cpp | Updates SSA diagnostics to use visitor pattern |
AndyAyersMS
approved these changes
Jul 16, 2025
|
Some somewhat substantial TP regressions (~0.5%). Detailed TP diff shows: Pushed a commit that mitigates it somewhat by introducing some slight duplication. There is now both a That mostly seems like changes in inlining, so I think we should take it as is. |
This was referenced Jul 16, 2025
|
/ba-g Android timeout and rest of failures are known |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace
GenTree::DefinesLocalby aGenTree::VisitLocalDefsto prepare for being able to allow a single node to have multiple definitions. Update uses to use the new function.I want to use this allow async calls to define a new value that represents whether the call suspended or not. This is necessary because runtime async calls should save and restore
Thread.CurrentThread._synchronizationContext, but the restore should only happen when the call finishes synchronously (with an exception or not). I need a proper representation in the JIT IR of these conditions to properly be able to model these conditions in the face of inlining.In a future change I expect to add a
LCL_ADDRnode as a new well-known argument to async calls that will be a definition that works similar to ret buffers. The async transformation will later expand the definition out in the resumption path.