Uh oh!
There was an error while loading. Please reload this page.
Use SSA-based TryGetRange to fold conditions - #129354
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR extends JIT assertion propagation to opportunistically fold ordered integer relational operators (<, <=, >, >=) to constant true/false using the SSA-based RangeCheck::TryGetRange analysis, and adds a small constant fast-path in TryGetRange itself to make such queries cheaper.
Changes:
- Add a constant fast-path to
RangeCheck::TryGetRangefor int/nint constants that fit inint32. - In
optAssertionPropGlobal_RelOp, when assertion-based range evaluation can’t prove a relop constant, attempt SSA-based operand range discovery viaTryGetRangeand fold when the relop becomes provably constant.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/jit/rangecheck.cpp | Adds a fast-path for constant expressions in RangeCheck::TryGetRange. |
| src/coreclr/jit/assertionprop.cpp | Adds SSA-based range evaluation to fold certain ordered relops during global assertion propagation. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
EgorBo
commented
Jun 14, 2026
/azp list |
This comment was marked as resolved.
This comment was marked as resolved.
EgorBo
commented
Jun 14, 2026
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr libraries-pgo, Fuzzlyn |
|
Azure Pipelines successfully started running 4 pipeline(s). |
…ed type The VNF_Cast handling in RangeCheck::GetRangeFromAssertionsWorker reused the source-type range for any widening cast, assuming the value is preserved. That is wrong when widening a signed source into a smaller-than-int unsigned type (e.g. (ushort)(sbyte)): negative source values are zero-extended into large positive values ((ushort)(-1) == 65535), so the source range no longer bounds the result. The new SSA-based relop folding then folded conditions such as 30997 > (ushort)s_F3 to a wrong constant. Use the destination-type range in that case. Found by Fuzzlyn. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
EgorBo
commented
Jun 15, 2026
/azp run Fuzzlyn |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Uh oh!
There was an error while loading. Please reload this page.
EgorBo
commented
Jun 15, 2026
PTAL @AndyAyersMS @dotnet/jit-contrib see description |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| assert(block != nullptr); | ||
| assert(tree != nullptr); | ||
| int budget = 64; |
There was a problem hiding this comment.
How do we arrive at these budget values?
There was a problem hiding this comment.
Purely SPMI driven. RangeCheck currently has a budget for all operations while this makes it per-operation, e.g. 50 has a bad impact on improvements, 100 has a bad impact on TP
EgorBo
commented
Jun 16, 2026
/azp run Fuzzlyn |
|
Azure Pipelines successfully started running 1 pipeline(s). |
In RangeCheck::GetRangeFromAssertionsWorker the range of a zero-extending (srcIsUnsigned) cast whose source is a signed small type (e.g. (uint)(sbyte), dumped as `CAST long <- ulong <- uint` of a TYP_BYTE) was taken from the unsigned small type, giving [0..255]. But small signed types are held sign-extended in their int-width slot, so (uint)(sbyte)(-1) == 0xFFFFFFFF, far outside [0..255]. Widen castFromType to the actual int source width so such casts fall back to an unknown range instead of an unsound small one. Fixes two pre-existing Fuzzlyn miscompiles where the bogus range made `3647 <= (uint)s` fold to false and a `% N` dividend look non-negative (signed long MOD converted to unsigned). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
BoyBaykiller
commented
Jun 16, 2026
With this we finally get rid of the branch here :) staticvoidTest(){for(inti=0;i<100;i++){if(i<0){Console.WriteLine("Hi");}}}G_M54612_IG02: ;; offset=0x0000moveax,99align[0 bytes for IG03] ;; size=5 bbWeight=0.25 PerfScore 0.06G_M54612_IG03: ;; offset=0x0005deceaxjne SHORT G_M54612_IG03 ;; size=4 bbWeight=4 PerfScore 5.00G_M54612_IG04: ;; offset=0x0009ret ;; size=1 bbWeight=1 PerfScore 1.00 |
EgorBo
commented
Jun 17, 2026
@AndyAyersMS I pushed another commit to fix a Fuzzlyn failure, but it likely was a pre-existing issue. But now it needs a new approval |
| var_types castFromType = srcIsUnsigned ? varTypeToUnsigned(arg0Typ) : arg0Typ; | ||
| if (genTypeSize(castFromType) < genTypeSize(castToType)) | ||
| // A zero-extending cast (srcIsUnsigned) of a signed sub-int source is unsound to bound by the |
There was a problem hiding this comment.
Why is srcIsUnsigned for int -> sbyte -> uint?
sbyte -> T is always sign extending, regardless of target type. While byte -> T is always zero extending.
So, I would expect that (uint)(sbyte)(-1) to say the src is signed, at each cast step.
While for (int)(byte)(-1) I'd expect the -1 -> byte to be srcIsSigned and byte -> int to be srcIsUnsigned
There was a problem hiding this comment.
sbyte -> T is always sign extending,
In that Fuzzlyn failure we end up with this:
[000040] OR long
[000039] --CXG+---U- CAST long <- ulong <- uint // U = GTF_UNSIGNED
[000038] --CXG+----- COMMA byte // s_19, an sbyte static field
...
[000034] IND byte (static Fseq[s_19]
castToType=ulong srcIsUnsigned=1 arg0Typ=sbyte
like you said, sbyte source but with srcIsUnsigned == true.
We probably should refactor casts a bit to make it clearer when to rely on what. Maybe we should do what LLVM does - separate sext and zest and separate nodes.
There was a problem hiding this comment.
Is that not a bug in VN or elsewhere here?
Presumably the actual issue is that we end up with VN information saying there is a zero-extension for a value that is signed and so we could end up with incorrect codegen in a few different ways.
There was a problem hiding this comment.
Yes, this change fixes #129516 and is not related to this PR, I fixed it because I didn't realize it was pre-existing on main and needed clean Fuzzlyn.
Is that not a bug in VN or elsewhere here?
I don't think so, it does look like a fallout artifact from enabling LONG for Ranges. srcIsUnsigned here isn't "the source is unsigned," it's the int -> long widening's extension mode VNF_Cast(byteVN, {long, │ srcIsUnsigned=true}) correctly means "zero-extend the 32-bit (sign-extended) operand," i.e. (long)(uint)(sbyte) .
Anyway, feel free to research it or if you believe it's not correct, for now it's a bug-fix for Fuzzlyn and judging by the diffs it doesn't make the impl conservative
There was a problem hiding this comment.
Talked about it on discord a bit. So in this case the source is unsigned, i.e. the cast represents an unsigned extension and so it will take something like 0x8000_0000 and produce 0x0000_0000_8000_0000, not 0xFFFF_FFFF_8000_0000
The general issue is that there's a secondary implicit cast from sbyte->int which isn't necessarily captured and some tools may overlook if they presume the castOp->TypeGet() is the actual source type. -- We probably should carefully audit this or maybe have some CastNode->CastOpType or similar metadata to avoid this issue and ensure that we see TYP_INT instead of TYP_BYTE or similar.
That is, we have something rather like 0x80 -> 0xFFFF_FF80 -> 0x0000_0000_FFFF_FF80 (sbyte->int->uint->ulong), and not 0x80 -> 0x0000_0080 -> 0x0000_0000_0000_0080 (byte->int->uint->ulong) like it was being interpreted as. -- Just for completeness sake, the other case it theoretically could've been was 0x80 -> 0xFFFF_FF80 -> 0xFFFF_FFFF_FFFF_FF80if it wassbyte->int->long`
EgorBo
commented
Jun 17, 2026
/ba-g timeouts |
Uh oh!
There was an error while loading. Please reload this page.
hez2010
commented
Jun 18, 2026
How about some SSA-based branch removal that can also figure out the loop ends up with a no-op then. |
EgorBo
commented
Jun 18, 2026
I think Jakob tried to implement a dead loop removal pass, but there were concerns regarding trivial dead endless loops |
jakobbotsch
commented
Jun 18, 2026
Yes, it was #103087 that we prototyped with @AndyAyersMS during the face to face a couple of years ago. |
jakobbotsch
commented
Jun 18, 2026
But this loop could be removed with a simple IV optimization. I think we need a different motivation than a random micro benchmark improving though, such dead loops seem to only be likely from benchmarks where it would just be annoying to do that optimization. |
[Diffs](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1462451&view=ms.vss-build-web.run-extensions-tab) (~ -1.2Mb on linux-x64) We have two ways of computing a `Range {int lo;int hi}` for a tree: * `GetRangeFromAssertions` (purely on top of VN and assertions, handles PHIs as well) * `TryGetRange` - SSA-based approach + assertions + `GetRangeFromAssertions`, used for GT_BOUNDS_CHECK only today. This PR enables `TryGetRange` for folding relops in global assertion prop. The TP Impact is 0.3-0.4% on average, I have some ideas how to improve it inside RangeCheck, but I think it's worth it. Mainly, it helps folding various Span.Slice checks we don't use GT_BOUNDS_CHECK for. NOTE: we don't set preferred bound yet when we call it, I'll experiment with that separately. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
[Diffs](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1462451&view=ms.vss-build-web.run-extensions-tab) (~ -1.2Mb on linux-x64) We have two ways of computing a `Range {int lo;int hi}` for a tree: * `GetRangeFromAssertions` (purely on top of VN and assertions, handles PHIs as well) * `TryGetRange` - SSA-based approach + assertions + `GetRangeFromAssertions`, used for GT_BOUNDS_CHECK only today. This PR enables `TryGetRange` for folding relops in global assertion prop. The TP Impact is 0.3-0.4% on average, I have some ideas how to improve it inside RangeCheck, but I think it's worth it. Mainly, it helps folding various Span.Slice checks we don't use GT_BOUNDS_CHECK for. NOTE: we don't set preferred bound yet when we call it, I'll experiment with that separately. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Diffs (~ -1.2Mb on linux-x64)
We have two ways of computing a
Range {int lo;int hi}for a tree:GetRangeFromAssertions(purely on top of VN and assertions, handles PHIs as well)TryGetRange- SSA-based approach + assertions +GetRangeFromAssertions, used for GT_BOUNDS_CHECK only today.This PR enables
TryGetRangefor folding relops in global assertion prop. The TP Impact is 0.3-0.4% on average, I have some ideas how to improve it inside RangeCheck, but I think it's worth it.Mainly, it helps folding various Span.Slice checks we don't use GT_BOUNDS_CHECK for.
NOTE: we don't set preferred bound yet when we call it, I'll experiment with that separately.