JIT: preserve local access type in relop assertion propagation - #134185
Conversation
|
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. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
🟢 Approval recommended
The fix and regression coverage address the reported JIT miscompilation.
Pull request overview
Fixes JIT assertion propagation by preserving the replacement local’s access type, preventing incorrect narrow loads and comparisons.
Changes:
- Preserves propagated local node types.
- Adds regression coverage for mismatched byte/int comparisons.
File summaries
| File | Description |
|---|---|
src/coreclr/jit/assertionprop.cpp |
Preserves the replacement local access type during relop propagation. |
src/tests/JIT/Regression_ro_2/Runtime_133859.cs |
Adds regression tests for issue #133859. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
|
PTAL @jakobbotsch, no diffs |
|
Ping @jakobbotsch |
|
What is the propagation that is happening here? Not totally sure that we would expect any propagation here in the first place given that it probably drops the normalize-on-store truncation that was happening implicitly? |
Simplify optAssertionPropGlobal_RelOp: inline optGlobalAssertionIsEqualOrNotEqual and fold a matching EQ/NE assertion straight into a constant instead of bashing op1 to a constant / retargeting its local and re-morphing. The local-retargeting path kept op1's small type, producing a truncated compare (dotnet#133859). Also drop the op1 side-effect/shape restrictions. Fixes dotnet#133859. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c20aa474-545f-4b8c-8143-b50e15f04910
13e46c2 to
08f6e54
Compare
|
@jakobbotsch I think it was actually correct, but I decided to just remove a bunch of code and inline // Bail out if op1 is not side effect free. Note we'll be bashing it below, unlike op2.
if ((op1->gtFlags & GTF_SIDE_EFFECT) != 0)
{
return nullptr;
}
if (!op1->OperIs(GT_LCL_VAR, GT_IND))
{
return nullptr;
}since we only work with VN there (global AP) and I deleted the path that relied on the o1 being side-effect free. |
Copy the replacement local's node type along with its local and SSA numbers. This allows the resulting self-comparison to fold instead of retaining a truncated load of an int local.
Fixes #133859.