JIT: enhance dead stores for locals where all uses are useasg #115031
Conversation
A local marked `GTF_VAR_USEASG` is not a true use, but a partial def. So if a tracked local has no true uses, then stores to that local are dead. In particular for local structs that are only stored to, those stores are dead.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
There was a problem hiding this comment.
Pull Request Overview
This PR refines dead store elimination by distinguishing true uses from partial assignments (useasg) for local variables.
- Introduces a new fgTrueUseSet to track genuine uses of a variable.
- Updates the marking of true uses in fgMarkUseDef and the liveness computation in fgComputeLifeTrackedLocalDef.
- Initializes the fgTrueUseSet per basic block in fgPerBlockLocalVarLiveness.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/jit/liveness.cpp | Adds logic to track and handle true uses and updates dead store elimination accordingly. |
| src/coreclr/jit/compiler.h | Declares the new fgTrueUseSet member to support the updated liveness analysis. |
Comments suppressed due to low confidence (1)
src/coreclr/jit/liveness.cpp:945
- Consider adding explicit unit tests to verify that variables with only GTF_VAR_USEASG definitions are correctly marked as dead stores.
isLive = VarSetOps::IsMember(this, fgTrueUseSet, varIndex) || VarSetOps::IsMember(this, keepAliveVars, varIndex);
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@jakobbotsch PTAL I recall seeing cases like this from object stack allocation but could not find them; perhaps we're better at promoting those fields now. At any rate, a decent handful of diffs, and this should be fairly cheap. |
PromotionLiveness does not consider partial defs as uses, so it is more precise around this. I tried making the same change to normal liveness back when I worked on physical promotion, but I ran into the issue that SSA depends on the use/def sets created by liveness and depends on partial defs being uses. This might have been the code, though it seems like too small of a diff so maybe I didn't push the full version: Perhaps your strategy here will work better, although it looks like there are some failures. |
|
Hmm, still not quite right. |
|
/azp run runtime-coreclr jitstress, runtime-coreclr pgostress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Jitstress failure is timeout in the stackoverflow tester (also seen failing elsewhere) |
|
@jakobbotsch think I've tracked down the last issues here. |
| { | ||
| // If the only uses are useasg defs, then the variable is not live unless artificially kept alive. | ||
| isLive = | ||
| VarSetOps::IsMember(this, fgTrueUseSet, varIndex) || VarSetOps::IsMember(this, keepAliveVars, varIndex); |
There was a problem hiding this comment.
If I understand right the fgTrueUseSet computed is a global set of variables and is not computed via data flow. Do you know how conservative that is, compared to making the data flow computation more precise?
I would be interesting in which liveness phase (early liveness, SSA's liveness or lowering's liveness) actually end up removing these stores. If it is not removed by SSA's liveness then I think we can do better by making the liveness computations more precise (similar to physical promotion's liveness).
There was a problem hiding this comment.
Just out of curiosity I opened a PR for my old change: #115081
There was a problem hiding this comment.
Spot checking I see some during SSA and some during Lower.
Let me add some metrics and see if I can get a more comprehensive picture.
There was a problem hiding this comment.
#115081 does seem to get most of the same diffs and a few more. Given that it does not require any new accounting I would be inclined to suggest that approach instead. I would also imagine it does not have any TP diffs once things are templated correctly, but that does require a bit of clean up.
There was a problem hiding this comment.
Fine by me. Are you going to finish off your version?
There was a problem hiding this comment.
Yeah, let me try to do that.
A local marked
GTF_VAR_USEASGis not a true use, but a partial def. So if a tracked local has no true uses, then stores to that local are dead.In particular for local structs that are only stored to, those stores are dead.