JIT: fix issue in redundant zero init opt #115698
Conversation
If there is a redundant field zero def followed by a nonzero USEASG def in a block with EH flow, redundant zero init will improperly mark the field as not def in the block, leading to an SSA assert when it processes the USEASG def and can't find a PHI in the handler. Treat USEASG defs as full defs. If analysis has reached this point then all fields are zero and so a full and partial def have the same effect. If the value being stored is not zero then accounting for the def (which will remain) prevents the optimization from removing any fields from the block def set. Fixes dotnet#115123.
github-actions
bot
added
the
area-CodeGen-coreclr
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a JIT optimizer bug where a redundant zero initialization could lead to missing PHI defs in EH flows when a nonzero USEASG def follows. It adds a repro test case and updates the optimizer logic to treat USEASG field stores as full defs.
- Add an Xunit test (
Runtime_115123.cs) that reproduces the SSA assert - Configure the test project to use optimizations (
.csproj) - Update
optRemoveRedundantZeroInitsto ignoreGTF_VAR_USEASGand treat all tracked-field writes as defs
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tests/JIT/Regression/JitBlue/Runtime_115123.csproj | Enable <Optimize>True</Optimize> for repro test |
| src/tests/JIT/Regression/JitBlue/Runtime_115123/Runtime_115123.cs | Add Problem(int a) test covering the JIT bug |
| src/coreclr/jit/optimizer.cpp | Remove GTF_VAR_USEASG guard so USEASG defs count as full defs |
Comments suppressed due to low confidence (1)
src/tests/JIT/Regression/JitBlue/Runtime_115123/Runtime_115123.cs:21
- The test only covers a == 1. To ensure the fix addresses all EH flow cases, consider parameterizing the test to run
Problem(a)for other values (e.g., 0, 2, 3, 4, and a default) so all switch branches and the zero/nonzero initialization paths are exercised.
Problem(1);
| // Here we treat both "full" and "partial" tracked field defs as defs | ||
| // (that is, we ignore the state of GTF_VAR_USEASG). |
There was a problem hiding this comment.
[nitpick] Consider clarifying this comment by removing the quotes around full/partial and explicitly stating that checks for GTF_VAR_USEASG are intentionally omitted to unify all tracked-field stores as definitions.
| // Here we treat both "full" and "partial" tracked field defs as defs | |
| // (that is, we ignore the state of GTF_VAR_USEASG). | |
| // Here we treat both full and partial tracked field definitions as definitions. | |
| // Checks for GTF_VAR_USEASG are intentionally omitted to unify all tracked-field stores as definitions. |
Copilot uses AI. Check for mistakes.
|
@jakobbotsch PTAL Seems like the block def set update for fields is a weak point for this opt. This is a minimalist fix. We could also try and beef up the per-field analysis in other ways. No diffs. |
It would in general be nice to avoid the dependency from SSA on the block def sets produced by liveness, but I'm not sure if it's doable without additional cost. |
|
Linux Arm64 failure is #115070, not sure why build analysis didn't pick it up. |
|
/ba-g known failure that build analysis didn't match. |
If there is a redundant field zero def followed by a nonzero USEASG def in a block with EH flow, redundant zero init will improperly mark the field as not def in the block, leading to an SSA assert when it processes the USEASG def and can't find a PHI in the handler.
Treat USEASG defs as full defs. If analysis has reached this point then all fields are zero and so if the second def value is zero, a full and partial def have the same effect. If the second def value is not zero then accounting for the def (which will remain) prevents the optimization from removing any of the fields from the block def set.
Fixes #115123.