From 92ce79366cedbfac1b0a7966c6843a89c3d989f4 Mon Sep 17 00:00:00 2001
From: Chris Fallin <chris@cfallin.org>
Date: Tue, 13 Dec 2022 11:47:20 -0800
Subject: [PATCH] riscv64: remove `valueregs_2_reg` extractor. (#5426)

This extractor had a side-effect of invoking `put_in_regs`, which is not
supposed to be invoked until the pattern-matching commits to evaluating
a rule right-hand side (i.e., cannot backtrack). In this case the
side-effect was mostly benign (in theory it could have caused additional
values to be computed needlessly), but in general we should be careful
to keep side-effects out of the left-hand side to enable further
optimizations and work on islec.

The implicit conversion from `Value` to `Reg` turns out to be enough to
make the rules in question work, so we can simply remove the use of the
extractor in this case.
---
 cranelift/codegen/src/isa/riscv64/inst.isle   |  5 --
 cranelift/codegen/src/isa/riscv64/lower.isle  | 58 +++++++++----------
 .../codegen/src/isa/riscv64/lower/isle.rs     |  4 --
 .../filetests/isa/riscv64/shift-rotate.clif   | 30 +++++-----
 4 files changed, 44 insertions(+), 53 deletions(-)

diff --git a/cranelift/codegen/src/isa/riscv64/inst.isle b/cranelift/codegen/src/isa/riscv64/inst.isle
index c6b9e1b794..e838533f81 100644
--- a/cranelift/codegen/src/isa/riscv64/inst.isle
+++ b/cranelift/codegen/src/isa/riscv64/inst.isle
@@ -1366,11 +1366,6 @@
   (load_imm12 x)
   (alu_rr_imm12 (AluOPRRI.Addi) (zero_reg) (imm12_const x)))
 
-;; Let me always get low part of ValueRegs.
-;; Sometimes I only need lowest bits, like `I8 << I128`.
-(decl valueregs_2_reg (Reg) Value)
-(extern extractor infallible valueregs_2_reg valueregs_2_reg)
-
 (decl lower_cls_i128 (ValueRegs) ValueRegs)
 (rule
   (lower_cls_i128 x)
diff --git a/cranelift/codegen/src/isa/riscv64/lower.isle b/cranelift/codegen/src/isa/riscv64/lower.isle
index b3a7ab3bd3..567f7e7c34 100644
--- a/cranelift/codegen/src/isa/riscv64/lower.isle
+++ b/cranelift/codegen/src/isa/riscv64/lower.isle
@@ -298,93 +298,93 @@
   (lower_popcnt_i128 x))
 
 ;;;; Rules for `ishl` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-(rule 1 (lower (has_type $I8 (ishl x (valueregs_2_reg y))))
-  (alu_rrr (AluOPRRR.Sllw) x (alu_andi y 7))
+(rule 1 (lower (has_type $I8 (ishl x y)))
+  (alu_rrr (AluOPRRR.Sllw) x (alu_andi (value_regs_get y 0) 7))
 )
 (rule 2 (lower (has_type $I8 (ishl x (imm12_from_value y))))
   (alu_rr_imm12 (AluOPRRI.Slliw) x (imm12_and y 7)))
 
-(rule 1 (lower (has_type $I16 (ishl x (valueregs_2_reg y))))
-  (alu_rrr (AluOPRRR.Sllw) x (alu_andi y 15))
+(rule 1 (lower (has_type $I16 (ishl x y)))
+  (alu_rrr (AluOPRRR.Sllw) x (alu_andi (value_regs_get y 0) 15))
 )
 (rule 2 (lower (has_type $I16 (ishl x (imm12_from_value y))))
   (alu_rr_imm12 (AluOPRRI.Slliw) x (imm12_and y 15)))
 
-(rule 1 (lower (has_type $I32 (ishl x (valueregs_2_reg y))))
-  (alu_rrr (AluOPRRR.Sllw) x y))
+(rule 1 (lower (has_type $I32 (ishl x y)))
+  (alu_rrr (AluOPRRR.Sllw) x (value_regs_get y 0)))
 (rule 2 (lower (has_type $I32 (ishl x (imm12_from_value y))))
   (alu_rr_imm12 (AluOPRRI.Slliw) x y))
 
 (rule 2 (lower (has_type $I64 (ishl x (imm12_from_value y))))
   (alu_rr_imm12 (AluOPRRI.Slli) x y))
-(rule 1 (lower (has_type $I64 (ishl x (valueregs_2_reg y))))
-  (alu_rrr (AluOPRRR.Sll) x y))
+(rule 1 (lower (has_type $I64 (ishl x y)))
+  (alu_rrr (AluOPRRR.Sll) x (value_regs_get y 0)))
 
 (rule 0 (lower (has_type $I128 (ishl x y)))
   (lower_i128_ishl x y))
 
 ;;;; Rules for `ushr` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-(rule 1 (lower (has_type $I8 (ushr x (valueregs_2_reg y))))
-  (alu_rrr (AluOPRRR.Srlw) (ext_int_if_need $false x $I8) (alu_andi y 7))
+(rule 1 (lower (has_type $I8 (ushr x y)))
+  (alu_rrr (AluOPRRR.Srlw) (ext_int_if_need $false x $I8) (alu_andi (value_regs_get y 0) 7))
 )
 (rule 2 (lower (has_type $I8 (ushr x (imm12_from_value y))))
   (alu_rr_imm12 (AluOPRRI.SrliW) (ext_int_if_need $false x $I8) (imm12_and y 7)))
 
-(rule 1 (lower (has_type $I16 (ushr x (valueregs_2_reg y))))
-  (alu_rrr (AluOPRRR.Srlw) (ext_int_if_need $false x $I16) (alu_andi y 15))
+(rule 1 (lower (has_type $I16 (ushr x y)))
+  (alu_rrr (AluOPRRR.Srlw) (ext_int_if_need $false x $I16) (alu_andi (value_regs_get y 0) 15))
 )
 (rule 2 (lower (has_type $I16 (ushr x (imm12_from_value y))))
   (alu_rr_imm12 (AluOPRRI.SrliW) (ext_int_if_need $false x $I16) (imm12_and y 15)))
 
-(rule 1 (lower (has_type $I32 (ushr x (valueregs_2_reg y))))
-  (alu_rrr (AluOPRRR.Srlw) x y))
+(rule 1 (lower (has_type $I32 (ushr x y)))
+  (alu_rrr (AluOPRRR.Srlw) x (value_regs_get y 0)))
 (rule 2 (lower (has_type $I32 (ushr x (imm12_from_value y))))
   (alu_rr_imm12 (AluOPRRI.SrliW) x y))
 
 (rule 2 (lower (has_type $I64 (ushr x (imm12_from_value y))))
   (alu_rr_imm12 (AluOPRRI.Srli) x y))
-(rule 1 (lower (has_type $I64 (ushr x (valueregs_2_reg y))))
-  (alu_rrr (AluOPRRR.Srl) x y))
+(rule 1 (lower (has_type $I64 (ushr x y)))
+  (alu_rrr (AluOPRRR.Srl) x (value_regs_get y 0)))
 
 (rule 0 (lower (has_type $I128 (ushr x y)))
   (lower_i128_ushr x y))
 
 
 ;;;; Rules for `sshr` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-(rule 1 (lower (has_type $I8 (sshr x (valueregs_2_reg y))))
-  (alu_rrr (AluOPRRR.Sra) (ext_int_if_need $true x $I8) (alu_andi y 7))
+(rule 1 (lower (has_type $I8 (sshr x y)))
+  (alu_rrr (AluOPRRR.Sra) (ext_int_if_need $true x $I8) (alu_andi (value_regs_get y 0) 7))
 )
 (rule 2 (lower (has_type $I8 (sshr x (imm12_from_value y))))
   (alu_rr_imm12 (AluOPRRI.Srai) (ext_int_if_need $true x $I8) (imm12_and y 7)))
 
-(rule 1 (lower (has_type $I16 (sshr x (valueregs_2_reg y))))
-  (alu_rrr (AluOPRRR.Sra) (ext_int_if_need $true x $I16) (alu_andi y 15))
+(rule 1 (lower (has_type $I16 (sshr x y)))
+  (alu_rrr (AluOPRRR.Sra) (ext_int_if_need $true x $I16) (alu_andi (value_regs_get y 0) 15))
 )
 (rule 2 (lower (has_type $I16 (sshr x (imm12_from_value y))))
   (alu_rr_imm12 (AluOPRRI.Srai) (ext_int_if_need $true x $I16) (imm12_and y 15)))
 
-(rule 1 (lower (has_type $I32 (sshr x (valueregs_2_reg y))))
-  (alu_rrr (AluOPRRR.Sraw) x y))
+(rule 1 (lower (has_type $I32 (sshr x y)))
+  (alu_rrr (AluOPRRR.Sraw) x (value_regs_get y 0)))
 (rule 2 (lower (has_type $I32 (sshr x (imm12_from_value y))))
   (alu_rr_imm12 (AluOPRRI.Sraiw) x y))
-(rule 1 (lower (has_type $I64 (sshr x (valueregs_2_reg y))))
-  (alu_rrr (AluOPRRR.Sra) x y))
+(rule 1 (lower (has_type $I64 (sshr x y)))
+  (alu_rrr (AluOPRRR.Sra) x (value_regs_get y 0)))
 (rule 2 (lower (has_type $I64 (sshr x (imm12_from_value y))))
   (alu_rr_imm12 (AluOPRRI.Srai) x y))
 (rule 0 (lower (has_type $I128 (sshr x y)))
-  (lower_i128_sshr x y))
+  (lower_i128_sshr x (value_regs_get y 0)))
 
 
 ;;;; Rules for `rotl` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-(rule (lower (has_type (fits_in_64 ty) (rotl x (valueregs_2_reg y))))
-  (lower_rotl ty (ext_int_if_need $false x ty) y))
+(rule (lower (has_type (fits_in_64 ty) (rotl x y)))
+  (lower_rotl ty (ext_int_if_need $false x ty) (value_regs_get y 0)))
 
 (rule 1 (lower (has_type $I128 (rotl x y)))
   (lower_i128_rotl x y))
 
 ;;;; Rules for `rotr` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-(rule (lower (has_type (fits_in_64 ty) (rotr x (valueregs_2_reg y))))
-  (lower_rotr ty (ext_int_if_need $false x ty) y))
+(rule (lower (has_type (fits_in_64 ty) (rotr x y)))
+  (lower_rotr ty (ext_int_if_need $false x ty) (value_regs_get y 0)))
 
 (rule 1 (lower (has_type $I128 (rotr x y)))
   (lower_i128_rotr x y))
diff --git a/cranelift/codegen/src/isa/riscv64/lower/isle.rs b/cranelift/codegen/src/isa/riscv64/lower/isle.rs
index 2b3df718da..caee8fed59 100644
--- a/cranelift/codegen/src/isa/riscv64/lower/isle.rs
+++ b/cranelift/codegen/src/isa/riscv64/lower/isle.rs
@@ -315,10 +315,6 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6>
         self.isa_flags.has_zbkb()
     }
 
-    fn valueregs_2_reg(&mut self, val: Value) -> Reg {
-        self.put_in_regs(val).regs()[0]
-    }
-
     fn inst_output_get(&mut self, x: InstOutput, index: u8) -> ValueRegs {
         x[index as usize]
     }
diff --git a/cranelift/filetests/filetests/isa/riscv64/shift-rotate.clif b/cranelift/filetests/filetests/isa/riscv64/shift-rotate.clif
index f6b224beb3..e1e4839999 100644
--- a/cranelift/filetests/filetests/isa/riscv64/shift-rotate.clif
+++ b/cranelift/filetests/filetests/isa/riscv64/shift-rotate.clif
@@ -364,13 +364,13 @@ block0(v0: i32):
 }
 
 ; block0:
-;   li t2,17
-;   uext.w a1,a0
-;   andi a3,t2,31
+;   uext.w t2,a0
+;   li a1,17
+;   andi a3,a1,31
 ;   li a5,32
 ;   sub a7,a5,a3
-;   sll t4,a1,a3
-;   srl t1,a1,a7
+;   sll t4,t2,a3
+;   srl t1,t2,a7
 ;   select_reg a0,zero,t1##condition=(a3 eq zero)
 ;   or a0,t4,a0
 ;   ret
@@ -383,13 +383,13 @@ block0(v0: i16):
 }
 
 ; block0:
-;   li t2,10
-;   uext.h a1,a0
-;   andi a3,t2,15
+;   uext.h t2,a0
+;   li a1,10
+;   andi a3,a1,15
 ;   li a5,16
 ;   sub a7,a5,a3
-;   sll t4,a1,a3
-;   srl t1,a1,a7
+;   sll t4,t2,a3
+;   srl t1,t2,a7
 ;   select_reg a0,zero,t1##condition=(a3 eq zero)
 ;   or a0,t4,a0
 ;   ret
@@ -402,13 +402,13 @@ block0(v0: i8):
 }
 
 ; block0:
-;   li t2,3
-;   uext.b a1,a0
-;   andi a3,t2,7
+;   uext.b t2,a0
+;   li a1,3
+;   andi a3,a1,7
 ;   li a5,8
 ;   sub a7,a5,a3
-;   sll t4,a1,a3
-;   srl t1,a1,a7
+;   sll t4,t2,a3
+;   srl t1,t2,a7
 ;   select_reg a0,zero,t1##condition=(a3 eq zero)
 ;   or a0,t4,a0
 ;   ret