From 4672280f645124c45074ed2ac026d34eb4ab5d92 Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Tue, 27 Feb 2024 14:54:01 -0800 Subject: [PATCH] cranelift: Fix ireduce rules 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 Co-authored-by: L Pereira Co-authored-by: Chris Fallin --- cranelift/codegen/src/opts/extends.isle | 53 +++++-------------------- cranelift/codegen/src/prelude_opt.isle | 12 ------ 2 files changed, 10 insertions(+), 55 deletions(-) diff --git a/cranelift/codegen/src/opts/extends.isle b/cranelift/codegen/src/opts/extends.isle index 268407b0748a..47ccfbf1647a 100644 --- a/cranelift/codegen/src/opts/extends.isle +++ b/cranelift/codegen/src/opts/extends.isle @@ -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))) diff --git a/cranelift/codegen/src/prelude_opt.isle b/cranelift/codegen/src/prelude_opt.isle index 1f02387d21f3..e0cd14860e88 100644 --- a/cranelift/codegen/src/prelude_opt.isle +++ b/cranelift/codegen/src/prelude_opt.isle @@ -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))))