ISLE opts for x ± a == y ± b
#10489
Merged
ISLE opts for x ± a == y ± b
#10489
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
scottmcm
commented
Mar 30, 2025
Comment on lines
-138
to
+140
| ;; v70 = iconst.i64 -1 | ||
| ;; @003b v50 = iadd v49, v70 ; v70 = -1 | ||
| ;; v71 = iconst.i64 0 | ||
| ;; @003b v51 = icmp eq v50, v71 ; v71 = 0 | ||
| ;; @003b brif v51, block5, block6 | ||
| ;; v93 = iconst.i64 1 | ||
| ;; v83 = icmp eq v49, v93 ; v93 = 1 | ||
| ;; @003b brif v83, block5, block6 |
There was a problem hiding this comment.
A case in the wild, here: was a + -1 == 0, now a == 1.
scottmcm
commented
Mar 30, 2025
Thinking more, it doesn't seem worth it to just change `x + 1 == y` to `x == y - 1` *always*.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 13, 2025
Allow matching on 3+ variant niche-encoded enums to optimize better
While the two-variant case is most common (and already special-cased), it's pretty unusual to actually need the *fully-general* niche-decoding algorithm (that handles things like 200+ variants wrapping the encoding space and such).
Layout puts the niche-encoded variants on one end of the natural values, so because enums don't have that many variants, it's quite common that there's no wrapping because the handful of variants just end up after the end of the `bool` or `char` or `newtype_index!` or whatever.
This PR thus looks for those cases: situations where the tag's range doesn't actually wrap, and thus we can check for niche-vs-untag in one simple `icmp` without needing to adjust the tag value, and by picking between zero- and sign-extension based on *which* kind of non-wrapping it is, also help LLVM better understand by not forcing it to think about wrapping arithmetic either.
It also emits the operations in a more optimization-friendly order. While the MIR Rvalue calculates a discriminant, so that's what we emit, code normally doesn't actually care about the actual discriminant for these niche-encoded enums. Rather, the discriminant is just getting passed to an equality check (for something like `matches!(foo, TerminatorKind::Goto { .. }`) or a `SwitchInt` (when it's being matched on).
So while the old code would emit, roughly
```rust
if is_niche { tag + ADJUSTMENT } else { UNTAGGED_DISCR }
```
this PR changes it instead to
```rust
(if is_niche { tag } else { UNTAGGED_ADJ_DISCR }) + ADJUSTMENT
```
which on its own might seem odd, but it's actually easier to optimize because what we're actually doing is
```rust
complicated_stuff() + ADJUSTMENT == 4
```
or
```rust
match complicated_stuff() + ADJUSTMENT { 0 =>…, 1 => …, 2 => …, _ => unreachable }
```
or in the generated `PartialEq` for enums with fieldless variants,
```rust
complicated_stuff(a) + ADJUSTMENT == complicated_stuff(b) + ADJUSTMENT
```
and thus that's easy for the optimizer to eliminate the additions:
```rust
complicated_stuff() == 2
```
```rust
match complicated_stuff() { 7 => …, 8 => …, 9 => …, _ => unreachable }
```
```rust
complicated_stuff(a) == complicated_stuff(b)
```
For good measure I went and made sure that cranelift can do this optimization too 🙂 bytecodealliance/wasmtime#10489
r? WaffleLapkin
Follow-up to rust-lang#139098
--
EDIT later: I happened to notice rust-lang#110197 (comment) -- it looks like there used to be some optimizations in this code, but they got removed for being wrong. I've added lots of tests here; let's hope I can avoid that fate 😬
(Certainly it would be possible to save some complexity by restricting this to the easy case, where it's unsigned-nowrap, the niches are after the natural payload, and all the variant indexes are small.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Noticed these looking at niched discriminant calculations, because
discr(a) == discr(b)sometimes expands tofrobble(a) + OFFSET == frobble(b) + OFFSET, where it's quite helpful to simplify tofrobble(a) == frobble(b)instead.