Skip to content

Commit

Permalink
Backport #8005 to release-18.0.0 (#8008)
Browse files Browse the repository at this point in the history
* cranelift: Fix ireduce rules (#8005)

We had two optimization rules which started off like this:

(rule (simplify (ireduce smallty val@(binary_op _ op x y)))
      (if-let _ (reducible_modular_op val))
      ...)

This was intended to check that `x` and `y` came from an instruction
which not only was a binary op but also matched `reducible_modular_op`.

Unfortunately, both `binary_op` and `reducible_modular_op` were
multi-terms.
- So `binary_op` would search the eclass rooted at `val` to find each
  instruction that uses a binary operator.
- Then `reducible_modular_op` would search the entire eclass again to
  find an instruction which matched its criteria.

Nothing ensured that both searches would find the same instruction.

The reason these rules were written this way was because they had
additional guards (`will_simplify_with_ireduce`) which made them fairly
complex, and it seemed desirable to not have to copy those guards for
every operator where we wanted to apply this optimization.

However, we've decided that checking whether the rule is actually an
improvement is not desirable. In general, that should be the job of the
cost function. Blindly adding equivalent expressions gives us more
opportunities for other rules to fire, and we have global recursion and
growth limits to keep the process from going too wild.

As a result, we can just delete those guards. That allows us to write
the rules in a more straightforward way.

Fixes #7999.

Co-authored-by: Trevor Elliott <telliott@fastly.com>
Co-authored-by: L Pereira <l.pereira@fastly.com>
Co-authored-by: Chris Fallin <chris@cfallin.org>

* Update RELEASES.md

* Update RELEASES.md

---------

Co-authored-by: Jamey Sharp <jsharp@fastly.com>
Co-authored-by: L Pereira <l.pereira@fastly.com>
Co-authored-by: Chris Fallin <chris@cfallin.org>
  • Loading branch information
4 people committed Feb 28, 2024
1 parent 446862c commit dfdd28e
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 55 deletions.
10 changes: 10 additions & 0 deletions RELEASES.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
--------------------------------------------------------------------------------

## 18.0.2

### Fixed

* Fix an egraph rule bug that was permitting an incorrect `ireduce` rewrite to
unary and binary operations, leading to miscompilations.
[#8005](https://github.com/bytecodealliance/wasmtime/pull/8005)

--------------------------------------------------------------------------------

## 18.0.1

Released 2024-02-20.
Expand Down
53 changes: 10 additions & 43 deletions cranelift/codegen/src/opts/extends.isle
Original file line number Diff line number Diff line change
Expand Up @@ -75,50 +75,17 @@
(rule (simplify (bxor bigty (uextend _ x@(value_type smallty)) (uextend _ y@(value_type smallty))))
(uextend bigty (bxor smallty x y)))

;; Matches values where `ireducing` them will not actually introduce another
;; instruction, since other rules will collapse them with the reduction.
(decl pure multi will_simplify_with_ireduce_rec (u8 Value) Value)
(rule (will_simplify_with_ireduce_rec _ x@(uextend _ _)) x)
(rule (will_simplify_with_ireduce_rec _ x@(sextend _ _)) x)
(rule (will_simplify_with_ireduce_rec _ x@(iconst _ _)) x)
(rule (will_simplify_with_ireduce_rec depth x@(unary_op _ _ a))
(if-let _ (u8_lt 0 depth))
(if-let _ (reducible_modular_op x))
(if-let _ (will_simplify_with_ireduce_rec (u8_sub depth 1) a))
x)
(rule (will_simplify_with_ireduce_rec depth x@(binary_op _ _ a b))
(if-let _ (u8_lt 0 depth))
(if-let _ (reducible_modular_op x))
(if-let _ (will_simplify_with_ireduce_rec (u8_sub depth 1) a))
(if-let _ (will_simplify_with_ireduce_rec (u8_sub depth 1) b))
x)

(decl pure multi will_simplify_with_ireduce (Value) Value)
(rule (will_simplify_with_ireduce x)
(will_simplify_with_ireduce_rec 2 x))

;; Replace `(small)(x OP y)` with `(small)x OP (small)y` in cases where that's
;; legal.
;; Matches values where the high bits of the input don't affect lower bits of
;; the output, and thus the inputs can be reduced before the operation rather
;; than doing the wide operation then reducing afterwards.
(decl pure multi reducible_modular_op (Value) Value)
(rule (reducible_modular_op x@(ineg _ _)) x)
(rule (reducible_modular_op x@(bnot _ _)) x)
(rule (reducible_modular_op x@(iadd _ _ _)) x)
(rule (reducible_modular_op x@(isub _ _ _)) x)
(rule (reducible_modular_op x@(imul _ _ _)) x)
(rule (reducible_modular_op x@(bor _ _ _)) x)
(rule (reducible_modular_op x@(bxor _ _ _)) x)
(rule (reducible_modular_op x@(band _ _ _)) x)
(rule (simplify (ireduce ty (ineg _ x))) (ineg ty (ireduce ty x)))
(rule (simplify (ireduce ty (bnot _ x))) (bnot ty (ireduce ty x)))

;; Replace `(small)(x OP y)` with `(small)x OP (small)y` in cases where that's
;; legal and it reduces the total number of instructions since the reductions
;; to the arguments simplify further.
(rule (simplify (ireduce smallty val@(unary_op _ op x)))
(if-let _ (reducible_modular_op val))
(if-let _ (will_simplify_with_ireduce x))
(unary_op smallty op (ireduce smallty x)))
(rule (simplify (ireduce smallty val@(binary_op _ op x y)))
(if-let _ (reducible_modular_op val))
(if-let _ (will_simplify_with_ireduce x))
(if-let _ (will_simplify_with_ireduce y))
(binary_op smallty op (ireduce smallty x) (ireduce smallty y)))
(rule (simplify (ireduce ty (iadd _ x y))) (iadd ty (ireduce ty x) (ireduce ty y)))
(rule (simplify (ireduce ty (isub _ x y))) (isub ty (ireduce ty x) (ireduce ty y)))
(rule (simplify (ireduce ty (imul _ x y))) (imul ty (ireduce ty x) (ireduce ty y)))
(rule (simplify (ireduce ty (bor _ x y))) (bor ty (ireduce ty x) (ireduce ty y)))
(rule (simplify (ireduce ty (bxor _ x y))) (bxor ty (ireduce ty x) (ireduce ty y)))
(rule (simplify (ireduce ty (band _ x y))) (band ty (ireduce ty x) (ireduce ty y)))
12 changes: 0 additions & 12 deletions cranelift/codegen/src/prelude_opt.isle
Original file line number Diff line number Diff line change
Expand Up @@ -120,15 +120,3 @@
(extractor (sextend_maybe ty val) (sextend_maybe_etor ty val))
(rule 0 (sextend_maybe ty val) (sextend ty val))
(rule 1 (sextend_maybe ty val@(value_type ty)) val)

(decl unary_op (Type Opcode Value) Value)
(extractor (unary_op ty opcode x)
(inst_data ty (InstructionData.Unary opcode x)))
(rule (unary_op ty opcode x)
(make_inst ty (InstructionData.Unary opcode x)))

(decl binary_op (Type Opcode Value Value) Value)
(extractor (binary_op ty opcode x y)
(inst_data ty (InstructionData.Binary opcode (value_array_2 x y))))
(rule (binary_op ty opcode x y)
(make_inst ty (InstructionData.Binary opcode (value_array_2_ctor x y))))

0 comments on commit dfdd28e

Please sign in to comment.