Skip to content

[Cranelift] `(n < m) → ((if c then m else x) < (if c then n else x)) … #12001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+35 −0
Open

[Cranelift] `(n < m) → ((if c then m else x) < (if c then n else x)) … #12001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions cranelift/codegen/src/opts/selects.isle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,11 @@
(rule (simplify (select ty d a (select ty d _ y))) (select ty d a y))
(rule (simplify (select ty d (select ty d x _) a)) (select ty d x a))

;; (n < m) → ((if c then m else x) < (if c then n else x)) = false
(rule
(simplify (slt cty
(select ty c (iconst_s ty z) x)
(select ty c (iconst_s ty y) x)))
(if-let true (i64_lt y z))
(iconst_u cty 0))
Comment on lines +105 to +111
Copy link
Member

@fitzgen fitzgen Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to add a general rule to dedupe selects, something like this:

(rule (simplify (slt ty (select _ cond a b)
                        (select _ cond c d)))
      (select ty cond (slt ty cond a c)
                      (slt ty cond b d)))

I think this could be an intermediate step that would reveal optimization possibilities for existing small rules and effectively subsume this larger rule. I think this is also beneficial on its own, since selects should generally be more expensive than slts (although I am not sure that our cost functions encode that at the moment), so I'm not worried about unnecessarily blowing up the enode count in this case.

All that said, we would really want the equivalent of this rule for ~all operators, not just slt:

(rule (simplify (iadd ty (select _ cond a b)
                         (select _ cond c d)))
      (select ty cond (iadd ty cond a c)
                      (iadd ty cond b d)))

And all those rules would be annoying to write in ISLE today without macros or higher-order terms.

But then again, roughly the same could be said about this rule as-is (it is combining a cprop rule, a x < x ==> false rule, and the pull-selects-out rule I sketched above; we could do the same kind of thing for all other operators' rules by combining them with their own version of the pull-selects-out rule).

So after writing all this out, I think I have convinced myself that my proposed intermediate rule is the way to go, rather than writing out the "combined" rule as you have here. (And we don't need to add all the other operator variants of that rule now, but probably should eventually.) But we should check that adding that rule really is enough to do the "combined" rewrite you've proposed in this PR. We should be able to check that via your existing tests.

Does all that make sense?

Copy link
Member

@fitzgen fitzgen Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(although I am not sure that our cost functions encode that at the moment)

#12006


15 changes: 15 additions & 0 deletions cranelift/filetests/filetests/egraph/selects.clif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,18 @@ block0(v0: i8):
; return v8
; }

function %test_slt_select_consts(i8, i32) -> i8 fast {
block0(v0: i8, v1: i32):
v2 = iconst.i32 20
v3 = iconst.i32 10
v4 = select v0, v2, v1
v5 = select v0, v3, v1
v6 = icmp slt v4, v5
return v6
}

; function %test_slt_select_consts(i8, i32) -> i8 fast {
; block0(v0: i8, v1: i32):
; v7 = iconst.i8 0
; return v7 ; v7 = 0
; }
12 changes: 12 additions & 0 deletions cranelift/filetests/filetests/runtests/select.clif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,3 +441,15 @@ block0(v0: i64, v1: i64):
; run: %simplify_select_ult_ne_i64(7, 9) == 0
; run: %simplify_select_ult_ne_i64(3, 1) == 1
; run: %simplify_select_ult_ne_i64(-1, 42) == 1

function %test_slt_select_consts(i8, i32) -> i8 fast {
block0(v0: i8, v1: i32):
v2 = iconst.i32 20
v3 = iconst.i32 10
v4 = select v0, v2, v1
v5 = select v0, v3, v1
v6 = icmp slt v4, v5
return v6
}

; run: %test_slt_select_consts(3, 42) == 0
Loading