JIT: Model partial defs less conservatively in liveness when possible #115081
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
jakobbotsch
changed the title
|
Nice. Even a small TP improvement. |
There was a problem hiding this comment.
Pull Request Overview
This PR refines the liveness analysis in the JIT by modeling partial definitions more liberally outside SSA, allowing for better dead-code elimination.
- Introduces a template parameter for fgMarkUseDef to distinguish SSA from non-SSA liveness.
- Updates multiple call sites to explicitly specify the liveness mode (using either !lowered or false).
- Documents the new behavior in fgMarkUseDef and related liveness routines.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/coreclr/jit/liveness.cpp | Updates to fgMarkUseDef logic and its various callers to differentiate SSA from non-SSA behavior in partial definitions. |
| src/coreclr/jit/compiler.h | Declaration of the fgMarkUseDef template to enable the new liveness semantics. |
| const bool isUse = !isDef || ((tree->gtFlags & GTF_VAR_USEASG) != 0); | ||
| const bool isDef = ((tree->gtFlags & GTF_VAR_DEF) != 0); | ||
| const bool isFullDef = isDef && ((tree->gtFlags & GTF_VAR_USEASG) == 0); | ||
| const bool isUse = ssaLiveness ? !isFullDef : !isDef; |
There was a problem hiding this comment.
Verify that the logic for computing the use set aligns with the intended liveness semantics: in SSA mode, only non-full defs are considered uses, whereas in non-SSA mode any def should not be treated as a use.
Copilot uses AI. Check for mistakes.
| @@ -216,7 +236,7 @@ void Compiler::fgPerNodeLocalVarLiveness(GenTree* tree) | |||
| case GT_LCL_FLD: | |||
| case GT_STORE_LCL_VAR: | |||
| case GT_STORE_LCL_FLD: | |||
| fgMarkUseDef(tree->AsLclVarCommon()); | |||
| fgMarkUseDef<!lowered>(tree->AsLclVarCommon()); | |||
There was a problem hiding this comment.
Ensure that the expression '!lowered' correctly reflects the intended SSA versus non-SSA evaluation and that 'lowered' is reliably defined and meaningful in this context.
Copilot uses AI. Check for mistakes.
| @@ -2087,7 +2110,7 @@ void Compiler::fgInterBlockLocalVarLiveness() | |||
| for (GenTree* cur = stmt->GetTreeListEnd(); cur != nullptr;) | |||
| { | |||
| assert(cur->OperIsAnyLocal()); | |||
| bool isDef = ((cur->gtFlags & GTF_VAR_DEF) != 0) && ((cur->gtFlags & GTF_VAR_USEASG) == 0); | |||
| bool isDef = (cur->gtFlags & GTF_VAR_DEF) != 0; | |||
There was a problem hiding this comment.
Confirm that removing the additional check for GTF_VAR_USEASG when computing isDef in fgInterBlockLocalVarLiveness is intentional and consistent with the new non-SSA liveness semantics.
Copilot uses AI. Check for mistakes.
| @@ -6720,6 +6720,7 @@ class Compiler | |||
|
|
|||
| PhaseStatus fgEarlyLiveness(); | |||
|
|
|||
| template<bool ssaLiveness> | |||
There was a problem hiding this comment.
Consider adding inline documentation to clarify the expected values of ssaLiveness and its impact on liveness modeling, ensuring that its usage is clear to future maintainers.
Copilot uses AI. Check for mistakes.
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS |
|
/asp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn |
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Before this change a partial def
v.x = 123was modelled semantically something likev = v with x = 123.This matches how SSA models these definitions. However, it has the downside that it causes partial defs to be considered uses.
This PR changes things so that other liveness passes take a more liberal view, where partial defs are no longer considered full defs, but are then also not considered uses. This allows more dead-code elimination around these cases.
The liveness that runs during SSA is not changed; it still takes the old view.
Based on an old change I had lying around in https://github.com/dotnet/runtime/compare/main...jakobbotsch:runtime:partial-defs-allow-dying?expand=1 as an alternative to #115031