Repository: bytecodealliance/wasmtime
Pull request #7599 (2023-11-29 08:05:01)

Cranelift: Remove some unneeded `arithmetic.isle` patterns
I noticed that, thanks to const prop, a few of the patterns in `arithmetic.isle` aren't actually needed to keep the same behaviour.

This seems to have mostly lowered the register numbers in the affected tests, so it seems at least plausible.  But if it's expected that patterns don't don't rely on canonicalization and it's better to keep these duplicates, then please just close this PR.

(I needed to learn how to edit patterns and write+run tests against them for what I actually want to do, so this was a useful learning exercise for me whether it lands or not.)


github-actions[bot]: #### Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "isle"

Thus the following users have been cc'd because of the following labels:

* cfallin: isle
* fitzgen: isle

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

[Learn more.](https://github.com/bytecodealliance/subscribe-to-label-action)
</details>

Patch:
diff --git a/cranelift/codegen/src/opts/arithmetic.isle b/cranelift/codegen/src/opts/arithmetic.isle
index 6553b8674..bbb40dbf2 100644
--- a/cranelift/codegen/src/opts/arithmetic.isle
+++ b/cranelift/codegen/src/opts/arithmetic.isle
@@ -1,15 +1,14 @@
 ;; rewrites for integer and floating-point arithmetic
 ;; eg: `iadd`, `isub`, `ineg`, `imul`, `fadd`, `fsub`, `fmul`
 
-;; x+0 == 0+x == x.
+;; For commutative instructions, we depend on cprop.isle pushing immediates to
+;; the right, and thus only simplify patterns like `x+0`, not `0+x`.
+
+;; x+0 == x.
 (rule (simplify (iadd ty
                       x
                       (iconst ty (u64_from_imm64 0))))
       (subsume x))
-(rule (simplify (iadd ty
-                      (iconst ty (u64_from_imm64 0))
-                      x))
-      (subsume x))
 ;; x-0 == x.
 (rule (simplify (isub ty
                       x
@@ -39,33 +38,22 @@
 ;; x-x == 0.
 (rule (simplify (isub (fits_in_64 (ty_int ty)) x x)) (subsume (iconst ty (imm64 0))))
 
-;; x*1 == 1*x == x.
+;; x*1 == x.
 (rule (simplify (imul ty
                       x
                       (iconst ty (u64_from_imm64 1))))
       (subsume x))
-(rule (simplify (imul ty
-                      (iconst ty (u64_from_imm64 1))
-                      x))
-      (subsume x))
 
-;; x*0 == 0*x == 0.
+;; x*0 == 0.
 (rule (simplify (imul ty
                       _
                       zero @ (iconst ty (u64_from_imm64 0))))
       (subsume zero))
-(rule (simplify (imul ty
-                      zero @ (iconst ty (u64_from_imm64 0))
-                      _))
-      (subsume zero))
 
-;; x*-1 == -1*x == ineg(x).
+;; x*-1 == ineg(x).
 (rule (simplify (imul ty x (iconst ty c)))
       (if-let -1 (i64_sextend_imm64 ty c))
       (ineg ty x))
-(rule (simplify (imul ty (iconst ty c) x))
-      (if-let -1 (i64_sextend_imm64 ty c))
-      (ineg ty x))
 
 ;; (!x) + 1 == 1 + (!x) == !(x) - (-1) == ineg(x)
 (rule (simplify (iadd ty (bnot ty x) (iconst ty (u64_from_imm64 1))))
@@ -99,11 +87,9 @@
 ;; TODO: strength reduction: div to shifts
 ;; TODO: div/rem by constants -> magic multiplications
 
-;; x*2 == 2*x == x+x.
+;; x*2 == x+x.
 (rule (simplify (imul ty x (iconst _ (simm32 2))))
       (iadd ty x x))
-(rule (simplify (imul ty (iconst _ (simm32 2)) x))
-      (iadd ty x x))
 
 ;; x*c == x<<log2(c) when c is a power of two.
 ;; Note that the type of `iconst` must be the same as the type of `imul`,


Test patch:
diff --git a/cranelift/filetests/filetests/egraph/arithmetic.clif b/cranelift/filetests/filetests/egraph/arithmetic.clif
index 24d0d9c42..542fde629 100644
--- a/cranelift/filetests/filetests/egraph/arithmetic.clif
+++ b/cranelift/filetests/filetests/egraph/arithmetic.clif
@@ -55,6 +55,39 @@ block0(v0: i32):
     ; check: return v2
 }
 
+function %zero_plus(i32) -> i32 {
+block0(v0: i32):
+    v1 = iconst.i32 0
+    v2 = iadd v1, v0
+    return v2
+    ; check: return v0
+}
+
+function %zero_times(i32) -> i32 {
+block0(v0: i32):
+    v1 = iconst.i32 0
+    v2 = imul v1, v0
+    return v2
+    ; check: return v1
+}
+
+function %one_times(i32) -> i32 {
+block0(v0: i32):
+    v1 = iconst.i32 1
+    v2 = imul v1, v0
+    return v2
+    ; check: return v0
+}
+
+function %two_times(i32) -> i32 {
+block0(v0: i32):
+    v1 = iconst.i32 2
+    v2 = imul v1, v0
+    return v2
+    ; check: v6 = iadd v0, v0
+    ; check: return v6
+}
+
 function %mul_minus_one(i32) -> i32 {
 block0(v0: i32):
     v1 = iconst.i32 0xffff_ffff ; -1
@@ -69,8 +102,8 @@ block0(v0: i32):
     v1 = iconst.i32 0xffff_ffff ; -1
     v2 = imul v1, v0
     return v2
-    ; check: v3 = ineg v0
-    ; check: return v3
+    ; check: v4 = ineg v0
+    ; check: return v4
 }
 
 function %ineg_not_plus_one(i32) -> i32 {
diff --git a/cranelift/filetests/filetests/egraph/associative-and-commutative.clif b/cranelift/filetests/filetests/egraph/associative-and-commutative.clif
index 37bfa8da5..005a87d5f 100644
--- a/cranelift/filetests/filetests/egraph/associative-and-commutative.clif
+++ b/cranelift/filetests/filetests/egraph/associative-and-commutative.clif
@@ -39,9 +39,9 @@ block0(v0: i32, v1: i32):
     v6 = iadd v4, v5
     return v6
 ; check:  v7 = iadd v0, v1
-; nextln: iconst.i32 78
-; nextln: v16 = iadd v7, v14  ; v14 = 78
-; check:  return v16
+; nextln: v9 = iconst.i32 78
+; nextln: v15 = iadd v7, v9  ; v9 = 78
+; check:  return v15
 }
 
 function %imul_shallow_and_wide(i32, i32, i32, i32) -> i32 {
diff --git a/cranelift/filetests/filetests/egraph/cprop.clif b/cranelift/filetests/filetests/egraph/cprop.clif
index e9fef8ca8..6de9aa695 100644
--- a/cranelift/filetests/filetests/egraph/cprop.clif
+++ b/cranelift/filetests/filetests/egraph/cprop.clif
@@ -9,8 +9,8 @@ block0:
     return v2
 }
 
-; check: v9 = iconst.i8 41
-; nextln: return v9
+; check: v3 = iconst.i8 41
+; nextln: return v3
 
 function %f1() -> i16 {
 block0:
diff --git a/cranelift/filetests/filetests/egraph/remat.clif b/cranelift/filetests/filetests/egraph/remat.clif
index a1be556ce..5c98a0751 100644
--- a/cranelift/filetests/filetests/egraph/remat.clif
+++ b/cranelift/filetests/filetests/egraph/remat.clif
@@ -22,10 +22,9 @@ block2:
 ; check:      v2 = iadd v0, v1
 ; check:      brif v2, block1, block2
 ; check:   block1:
-; check:      v11 = iconst.i32 126
-; check:      v13 = iadd.i32 v0, v11
-; check:      return v13
+; check:      v6 = iconst.i32 126
+; check:      v12 = iadd.i32 v0, v6
+; check:      return v12
 ; check:   block2:
-; check:      v15 = iadd.i32 v0, v1
-; check:      return v15
-
+; check:      v14 = iadd.i32 v0, v1
+; check:      return v14
diff --git a/cranelift/filetests/filetests/egraph/shifts.clif b/cranelift/filetests/filetests/egraph/shifts.clif
index f2b992771..a9b321b20 100644
--- a/cranelift/filetests/filetests/egraph/shifts.clif
+++ b/cranelift/filetests/filetests/egraph/shifts.clif
@@ -651,9 +651,9 @@ block0(v0: i8):
     return v4
 }
 
-; check: v11 = iconst.i8 2
-; check: v13 = rotr v0, v11  ; v11 = 2
-; check: return v13
+; check: v6 = iconst.i8 2
+; check: v12 = rotr v0, v6  ; v6 = 2
+; check: return v12
 
 function %rotr_rotr_add(i8, i8, i8) -> i8 {
 block0(v0: i8, v1: i8, v2: i8):
@@ -675,9 +675,9 @@ block0(v0: i8):
     return v4
 }
 
-; check: v11 = iconst.i8 2
-; check: v13 = rotl v0, v11  ; v11 = 2
-; check: return v13
+; check: v6 = iconst.i8 2
+; check: v12 = rotl v0, v6  ; v6 = 2
+; check: return v12
 
 function %rotl_rotl_add(i8, i8, i8) -> i8 {
 block0(v0: i8, v1: i8, v2: i8):