Small clean up in assertionprop for relops #109987
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
PTAL @jakobbotsch @dotnet/jit-contrib PTAL, small diffs (mostly just a code cleanup) |
| fgMorphBlockStmt(block, newStmt DEBUGARG(__FUNCTION__)); | ||
| } | ||
|
|
||
| const bool evalsToTrue = (vnStore->CoercedConstantValue<INT64>(vnCns) != 0); |
There was a problem hiding this comment.
note: we already produce a lot of JTRUE(1) nodes when opts are enabled (they're not allowed in tier0 since lower might not expect it)
There was a problem hiding this comment.
I don't think we do. At this point in the JIT we have the invariant that the child of a JTRUE is always a relop.
Is this change making assertion prop produce JTRUE(non-relop) that other phases will see? That's changing IR invariants. The other opts phases might not handle that well (so specifically when JitOptRepeat is enabled). Lowering should handle it fine both with and without optimizations enabled as I removed the invariant from the backend in #82766.
Some phases (like morph when optimizing) temporarily introduce the pattern, but they also fold it away before the phase ends.
I would be fine with removing the invariant, but then the change should be explicit about the fact that it's changing the invariants, and I think we should do it more globally (e.g. validate that it's fine for all the opts phases).
There was a problem hiding this comment.
@jakobbotsch this code feeds morph so it is always folded, but ok, I've relaxed it back to 0==0. Looks like there is a room for improvement here, e.g. we refuse to fold branches where one of the constant has side-effects via COMMA
There was a problem hiding this comment.
If this is folded within assertion prop then the previous implementation was ok as well, but this one looks fine with me too.
| // Let's maintain the invariant that JTRUE's operand is always a relop. | ||
| // and if we have side effects, we wrap one of the operands with them, not the relop. | ||
| const bool evalsToTrue = (vnStore->CoercedConstantValue<INT64>(vnCns) != 0); | ||
| test->AsOp()->gtOp1 = gtNewOperNode(evalsToTrue ? GT_EQ : GT_NE, relop->TypeGet(), gtNewFalse(), gtNewFalse()); |
There was a problem hiding this comment.
Does it matter that this doesn't VN the new nodes? The old logic did.
There was a problem hiding this comment.
Does it matter that this doesn't VN the new nodes? The old logic did.
My understanding that it always invokes morph right after this and morph folds JTRUE(0==0) so it's not necessary. It also has no impact on diffs
|
Ignore format jit errors. Once zlib-ng/zlib-ng#1810 is reviewed/merged, I'll port the patch. |
This PR:
optVNConstantPropOnJTrueoptAssertionProp_RangePropertiesnow forarray[idx] = 42;tells thatarray.Lengthis never negative and never zero.