Simplify LowerBackend interface (#5432)

* Refactor lower_branch to have Unit result

Branches cannot have any output, so it is more straightforward
to have the ISLE term return Unit instead of InstOutput.

Also provide a new `emit_side_effect` term to simplify
implementation of `lower_branch` rules with Unit result.

* Simplify LowerBackend interface

Move all remaining asserts from the LowerBackend::lower and
::lower_branch_group into the common call site.

Change return value of ::lower to Option<InstOutput>, and
return value of ::lower_branch_group to Option<()> to match
ISLE term signature.

Only pass the first branch into ::lower_branch_group and
rename it to ::lower_branch.

As a result of all those changes, LowerBackend routines
now consists solely to calls to the corresponding ISLE
routines.

cranelift/codegen/src/isa/aarch64/inst.isle

@@ -3748,15 +3748,15 @@
        (MInst.EmitIsland needed_space)))
 
 ;; Helper for emitting `br_table` sequences.
-(decl br_table_impl (u64 Reg VecMachLabel) InstOutput)
+(decl br_table_impl (u64 Reg VecMachLabel) Unit)
 (rule (br_table_impl (imm12_from_u64 jt_size) ridx targets)
       (let ((jt_info BoxJTSequenceInfo (targets_jt_info targets)))
-       (side_effect (with_flags_side_effect
+       (emit_side_effect (with_flags_side_effect
             (cmp_imm (OperandSize.Size32) ridx jt_size)
             (jt_sequence ridx jt_info)))))
 (rule -1 (br_table_impl jt_size ridx targets)
       (let ((jt_size Reg (imm $I64 (ImmExtend.Zero) jt_size))
             (jt_info BoxJTSequenceInfo (targets_jt_info targets)))
-       (side_effect (with_flags_side_effect
+       (emit_side_effect (with_flags_side_effect
             (cmp (OperandSize.Size32) ridx jt_size)
             (jt_sequence ridx jt_info)))))

cranelift/codegen/src/isa/aarch64/lower.isle

@@ -12,7 +12,7 @@
 ;; blocks while we lower, the fallthrough in the new order is not (necessarily)
 ;; the same as the fallthrough in CLIF. So, we use the explicitly-provided
 ;; target.
-(decl partial lower_branch (Inst VecMachLabel) InstOutput)
+(decl partial lower_branch (Inst VecMachLabel) Unit)
 
 ;;;; Rules for `iconst` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
@@ -2422,7 +2422,7 @@
             (cond Cond (invert_cond (cond_code (flags_and_cc_cc comparison))))
             (taken BranchTarget (branch_target targets 0))
             (not_taken BranchTarget (branch_target targets 1)))
-        (side_effect
+        (emit_side_effect
          (with_flags_side_effect (flags_and_cc_flags comparison)
                                  (cond_br taken
                                           not_taken
@@ -2433,7 +2433,7 @@
             (cond Cond (cond_code (flags_and_cc_cc comparison)))
             (taken BranchTarget (branch_target targets 0))
             (not_taken BranchTarget (branch_target targets 1)))
-        (side_effect
+        (emit_side_effect
          (with_flags_side_effect (flags_and_cc_flags comparison)
                                  (cond_br taken
                                           not_taken
@@ -2444,7 +2444,7 @@
             (cond Cond (invert_cond cond)) ;; negate for `brz`
             (taken BranchTarget (branch_target targets 0))
             (not_taken BranchTarget (branch_target targets 1)))
-       (side_effect
+       (emit_side_effect
         (with_flags_side_effect (fpu_cmp (scalar_size ty) x y)
                                 (cond_br taken not_taken
                                  (cond_br_cond cond))))))
@@ -2453,7 +2453,7 @@
       (let ((cond Cond (fp_cond_code cc))
             (taken BranchTarget (branch_target targets 0))
             (not_taken BranchTarget (branch_target targets 1)))
-       (side_effect
+       (emit_side_effect
         (with_flags_side_effect (fpu_cmp (scalar_size ty) x y)
                                 (cond_br taken not_taken
                                  (cond_br_cond cond))))))
@@ -2466,7 +2466,7 @@
             (rt Reg (orr $I64 c_lo c_hi))
             (taken BranchTarget (branch_target targets 0))
             (not_taken BranchTarget (branch_target targets 1)))
-       (side_effect
+       (emit_side_effect
         (with_flags_side_effect flags
          (cond_br taken not_taken (cond_br_zero rt))))))
 (rule -2 (lower_branch (brz c @ (value_type ty) _ _) targets)
@@ -2475,7 +2475,7 @@
             (rt Reg (put_in_reg_zext64 c))
             (taken BranchTarget (branch_target targets 0))
             (not_taken BranchTarget (branch_target targets 1)))
-       (side_effect
+       (emit_side_effect
         (with_flags_side_effect flags
          (cond_br taken not_taken (cond_br_zero rt))))))
 ;; standard `brnz`
@@ -2487,7 +2487,7 @@
             (rt Reg (orr $I64 c_lo c_hi))
             (taken BranchTarget (branch_target targets 0))
             (not_taken BranchTarget (branch_target targets 1)))
-       (side_effect
+       (emit_side_effect
         (with_flags_side_effect flags
          (cond_br taken not_taken (cond_br_not_zero rt))))))
 (rule -2 (lower_branch (brnz c @ (value_type ty) _ _) targets)
@@ -2496,14 +2496,14 @@
             (rt Reg (put_in_reg_zext64 c))
             (taken BranchTarget (branch_target targets 0))
             (not_taken BranchTarget (branch_target targets 1)))
-       (side_effect
+       (emit_side_effect
         (with_flags_side_effect flags
          (cond_br taken not_taken (cond_br_not_zero rt))))))
 
 ;;; Rules for `jump` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (rule (lower_branch (jump _ _) targets)
-      (side_effect (aarch64_jump (branch_target targets 0))))
+      (emit_side_effect (aarch64_jump (branch_target targets 0))))
 
 ;;; Rules for `br_table` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 

cranelift/codegen/src/isa/aarch64/lower.rs

@@ -15,7 +15,6 @@ use crate::isa::aarch64::inst::*;
 use crate::isa::aarch64::AArch64Backend;
 use crate::machinst::lower::*;
 use crate::machinst::{Reg, Writable};
-use crate::CodegenResult;
 use crate::{machinst::*, trace};
 use smallvec::{smallvec, SmallVec};
 
@@ -745,51 +744,17 @@ pub(crate) fn maybe_value_multi(
 impl LowerBackend for AArch64Backend {
     type MInst = Inst;
 
-    fn lower(&self, ctx: &mut Lower<Inst>, ir_inst: IRInst) -> CodegenResult<InstOutput> {
+    fn lower(&self, ctx: &mut Lower<Inst>, ir_inst: IRInst) -> Option<InstOutput> {
-        if let Some(temp_regs) = super::lower::isle::lower(ctx, self, ir_inst) {
+        isle::lower(ctx, self, ir_inst)
-            return Ok(temp_regs);
-        }
-
-        let ty = if ctx.num_outputs(ir_inst) > 0 {
-            Some(ctx.output_ty(ir_inst, 0))
-        } else {
-            None
-        };
-
-        unreachable!(
-            "not implemented in ISLE: inst = `{}`, type = `{:?}`",
-            ctx.dfg().display_inst(ir_inst),
-            ty
-        );
     }
 
-    fn lower_branch_group(
+    fn lower_branch(
         &self,
         ctx: &mut Lower<Inst>,
-        branches: &[IRInst],
+        ir_inst: IRInst,
         targets: &[MachLabel],
-    ) -> CodegenResult<()> {
+    ) -> Option<()> {
-        // A block should end with at most two branches. The first may be a
+        isle::lower_branch(ctx, self, ir_inst, targets)
-        // conditional branch; a conditional branch can be followed only by an
-        // unconditional branch or fallthrough. Otherwise, if only one branch,
-        // it may be an unconditional branch, a fallthrough, a return, or a
-        // trap. These conditions are verified by `is_ebb_basic()` during the
-        // verifier pass.
-        assert!(branches.len() <= 2);
-        if branches.len() == 2 {
-            let op1 = ctx.data(branches[1]).opcode();
-            assert!(op1 == Opcode::Jump);
-        }
-
-        if let Some(temp_regs) = super::lower::isle::lower_branch(ctx, self, branches[0], targets) {
-            assert!(temp_regs.len() == 0);
-            return Ok(());
-        }
-
-        unreachable!(
-            "not implemented in ISLE: branch = `{}`",
-            ctx.dfg().display_inst(branches[0]),
-        );
     }
 
     fn maybe_pinned_reg(&self) -> Option<Reg> {

cranelift/codegen/src/isa/aarch64/lower/isle.rs

@@ -64,7 +64,7 @@ pub(crate) fn lower_branch(
     backend: &AArch64Backend,
     branch: Inst,
     targets: &[MachLabel],
-) -> Option<InstOutput> {
+) -> Option<()> {
     // TODO: reuse the ISLE context across lowerings so we can reuse its
     // internal heap allocations.
     let mut isle_ctx = IsleContext { lower_ctx, backend };

cranelift/codegen/src/isa/riscv64/inst.isle

@@ -1865,22 +1865,22 @@
 (decl vec_label_get (VecMachLabel u8) MachLabel )
 (extern constructor vec_label_get vec_label_get)
 
-(decl partial lower_branch (Inst VecMachLabel) InstOutput)
+(decl partial lower_branch (Inst VecMachLabel) Unit)
 (rule (lower_branch (jump _ _) targets )
-      (side_effect (SideEffectNoResult.Inst (gen_jump (vec_label_get targets 0)))))
+      (emit_side_effect (SideEffectNoResult.Inst (gen_jump (vec_label_get targets 0)))))
 
 ;;; cc a b targets Type
-(decl lower_br_icmp (IntCC ValueRegs ValueRegs VecMachLabel Type) InstOutput)
+(decl lower_br_icmp (IntCC ValueRegs ValueRegs VecMachLabel Type) Unit)
 (extern constructor lower_br_icmp lower_br_icmp)
 
-(decl lower_br_fcmp (FloatCC Reg Reg VecMachLabel Type) InstOutput)
+(decl lower_br_fcmp (FloatCC Reg Reg VecMachLabel Type) Unit)
 (extern constructor lower_br_fcmp lower_br_fcmp)
 
 ;; int scalar zero regs.
 (decl int_zero_reg (Type) ValueRegs)
 (extern constructor int_zero_reg int_zero_reg)
 
-(decl lower_brz_or_nz (IntCC ValueRegs VecMachLabel Type) InstOutput)
+(decl lower_brz_or_nz (IntCC ValueRegs VecMachLabel Type) Unit)
 (extern constructor lower_brz_or_nz lower_brz_or_nz)
 
 ;; Normalize a value for comparision.
@@ -1940,7 +1940,7 @@
   (lower_br_fcmp cc a b targets ty))
 
 ;;;
-(decl lower_br_table (Reg VecMachLabel) InstOutput)
+(decl lower_br_table (Reg VecMachLabel) Unit)
 (extern constructor lower_br_table lower_br_table)
 
 (rule

cranelift/codegen/src/isa/riscv64/lower.rs

@@ -4,7 +4,6 @@ use crate::isa::riscv64::inst::*;
 use crate::isa::riscv64::Riscv64Backend;
 use crate::machinst::lower::*;
 use crate::machinst::*;
-use crate::CodegenResult;
 pub mod isle;
 
 //=============================================================================
@@ -13,53 +12,17 @@ pub mod isle;
 impl LowerBackend for Riscv64Backend {
     type MInst = Inst;
 
-    fn lower(&self, ctx: &mut Lower<Inst>, ir_inst: IRInst) -> CodegenResult<InstOutput> {
+    fn lower(&self, ctx: &mut Lower<Inst>, ir_inst: IRInst) -> Option<InstOutput> {
-        if let Some(temp_regs) = super::lower::isle::lower(ctx, self, ir_inst) {
+        isle::lower(ctx, self, ir_inst)
-            return Ok(temp_regs);
-        }
-
-        let ty = if ctx.num_outputs(ir_inst) > 0 {
-            Some(ctx.output_ty(ir_inst, 0))
-        } else {
-            None
-        };
-
-        unreachable!(
-            "not implemented in ISLE: inst = `{}`, type = `{:?}`",
-            ctx.dfg().display_inst(ir_inst),
-            ty
-        );
     }
 
-    fn lower_branch_group(
+    fn lower_branch(
         &self,
         ctx: &mut Lower<Inst>,
-        branches: &[IRInst],
+        ir_inst: IRInst,
         targets: &[MachLabel],
-    ) -> CodegenResult<()> {
+    ) -> Option<()> {
-        // A block should end with at most two branches. The first may be a
+        isle::lower_branch(ctx, self, ir_inst, targets)
-        // conditional branch; a conditional branch can be followed only by an
-        // unconditional branch or fallthrough. Otherwise, if only one branch,
-        // it may be an unconditional branch, a fallthrough, a return, or a
-        // trap. These conditions are verified by `is_ebb_basic()` during the
-        // verifier pass.
-        assert!(branches.len() <= 2);
-        if branches.len() == 2 {
-            let op1 = ctx.data(branches[1]).opcode();
-            assert!(op1 == Opcode::Jump);
-        }
-
-        // Lower the first branch in ISLE.  This will automatically handle
-        // the second branch (if any) by emitting a two-way conditional branch.
-        if let Some(temp_regs) = super::lower::isle::lower_branch(ctx, self, branches[0], targets) {
-            assert!(temp_regs.len() == 0);
-            return Ok(());
-        }
-
-        unreachable!(
-            "not implemented in ISLE: branch = `{}`",
-            ctx.dfg().display_inst(branches[0]),
-        );
     }
 
     fn maybe_pinned_reg(&self) -> Option<Reg> {

cranelift/codegen/src/isa/riscv64/lower/isle.rs

@@ -68,7 +68,7 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Riscv64Backend> {
         b: Reg,
         targets: &VecMachLabel,
         ty: Type,
-    ) -> InstOutput {
+    ) -> Unit {
         let tmp = self.temp_writable_reg(I64);
         MInst::lower_br_fcmp(
             *cc,
@@ -81,7 +81,6 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Riscv64Backend> {
         )
         .iter()
         .for_each(|i| self.emit(i));
-        InstOutput::default()
     }
 
     fn lower_brz_or_nz(
@@ -90,7 +89,7 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Riscv64Backend> {
         a: ValueRegs,
         targets: &VecMachLabel,
         ty: Type,
-    ) -> InstOutput {
+    ) -> Unit {
         MInst::lower_br_icmp(
             *cc,
             a,
@@ -101,7 +100,6 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Riscv64Backend> {
         )
         .iter()
         .for_each(|i| self.emit(i));
-        InstOutput::default()
     }
     fn lower_br_icmp(
         &mut self,
@@ -110,7 +108,7 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Riscv64Backend> {
         b: ValueRegs,
         targets: &VecMachLabel,
         ty: Type,
-    ) -> InstOutput {
+    ) -> Unit {
         let test = generated_code::constructor_lower_icmp(self, cc, a, b, ty);
         self.emit(&MInst::CondBr {
             taken: BranchTarget::Label(targets[0]),
@@ -121,7 +119,6 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Riscv64Backend> {
                 rs2: zero_reg(),
             },
         });
-        InstOutput::default()
     }
     fn load_ra(&mut self) -> Reg {
         if self.backend.flags.preserve_frame_pointers() {
@@ -397,7 +394,7 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Riscv64Backend> {
         tmp.to_reg()
     }
 
-    fn lower_br_table(&mut self, index: Reg, targets: &VecMachLabel) -> InstOutput {
+    fn lower_br_table(&mut self, index: Reg, targets: &VecMachLabel) -> Unit {
         let tmp1 = self.temp_writable_reg(I64);
         let targets: Vec<BranchTarget> = targets
             .into_iter()
@@ -409,7 +406,6 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Riscv64Backend> {
             tmp1,
             targets,
         });
-        InstOutput::default()
     }
     fn x_reg(&mut self, x: u8) -> Reg {
         x_reg(x as usize)
@@ -446,7 +442,7 @@ pub(crate) fn lower_branch(
     backend: &Riscv64Backend,
     branch: Inst,
     targets: &[MachLabel],
-) -> Option<InstOutput> {
+) -> Option<()> {
     // TODO: reuse the ISLE context across lowerings so we can reuse its
     // internal heap allocations.
     let mut isle_ctx = IsleContext { lower_ctx, backend };

cranelift/codegen/src/isa/s390x/lower.isle

@@ -7,7 +7,7 @@
 ;; A variant of the main lowering constructor term, used for branches.
 ;; The only difference is that it gets an extra argument holding a vector
 ;; of branch targets to be used.
-(decl partial lower_branch (Inst VecMachLabel) InstOutput)
+(decl partial lower_branch (Inst VecMachLabel) Unit)
 
 
 ;;;; Rules for `iconst` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
@@ -3715,7 +3715,7 @@
 ;; Unconditional branch.  The target is found as first (and only) element in
 ;; the list of the current block's branch targets passed as `targets`.
 (rule (lower_branch (jump _ _) targets)
-      (side_effect (jump_impl (vec_element targets 0))))
+      (emit_side_effect (jump_impl (vec_element targets 0))))
 
 
 ;;;; Rules for `br_table` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
@@ -3731,8 +3731,8 @@
             (cond ProducesBool
               (bool (icmpu_uimm32 $I64 idx (vec_length_minus1 targets))
                     (intcc_as_cond (IntCC.UnsignedGreaterThanOrEqual))))
-            (_ InstOutput (side_effect (oneway_cond_br_bool cond
+            (_ Unit (emit_side_effect (oneway_cond_br_bool cond
-                                         (vec_element targets 0)))))
+                                        (vec_element targets 0)))))
         ;; Scale the index by the element size, and then emit the
         ;; compound instruction that does:
         ;;
@@ -3747,7 +3747,7 @@
         ;; PC-rel offset to the jumptable would be incorrect.
         ;; (The alternative is to introduce a relocation pass
         ;; for inlined jumptables, which is much worse, IMHO.)
-        (side_effect (jt_sequence (lshl_imm $I64 idx 2) targets))))
+        (emit_side_effect (jt_sequence (lshl_imm $I64 idx 2) targets))))
 
 
 ;;;; Rules for `brz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
@@ -3756,9 +3756,9 @@
 ;; - element 0: target if the condition is true (i.e. value is zero)
 ;; - element 1: target if the condition is false (i.e. value is nonzero)
 (rule (lower_branch (brz val_cond _ _) targets)
-      (side_effect (cond_br_bool (invert_bool (value_nonzero val_cond))
+      (emit_side_effect (cond_br_bool (invert_bool (value_nonzero val_cond))
-                                 (vec_element targets 0)
+                                      (vec_element targets 0)
-                                 (vec_element targets 1))))
+                                      (vec_element targets 1))))
 
 
 ;;;; Rules for `brnz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
@@ -3767,9 +3767,9 @@
 ;; - element 0: target if the condition is true (i.e. value is nonzero)
 ;; - element 1: target if the condition is false (i.e. value is zero)
 (rule (lower_branch (brnz val_cond _ _) targets)
-      (side_effect (cond_br_bool (value_nonzero val_cond)
+      (emit_side_effect (cond_br_bool (value_nonzero val_cond)
-                                 (vec_element targets 0)
+                                     (vec_element targets 0)
-                                 (vec_element targets 1))))
+                                     (vec_element targets 1))))
 
 
 ;;;; Rules for `trap` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

cranelift/codegen/src/isa/s390x/lower.rs

@@ -1,11 +1,9 @@
 //! Lowering rules for S390x.
 
 use crate::ir::Inst as IRInst;
-use crate::ir::Opcode;
 use crate::isa::s390x::inst::Inst;
 use crate::isa::s390x::S390xBackend;
 use crate::machinst::{InstOutput, Lower, LowerBackend, MachLabel};
-use crate::CodegenResult;
 
 pub mod isle;
 
@@ -15,52 +13,16 @@ pub mod isle;
 impl LowerBackend for S390xBackend {
     type MInst = Inst;
 
-    fn lower(&self, ctx: &mut Lower<Inst>, ir_inst: IRInst) -> CodegenResult<InstOutput> {
+    fn lower(&self, ctx: &mut Lower<Inst>, ir_inst: IRInst) -> Option<InstOutput> {
-        if let Some(temp_regs) = super::lower::isle::lower(ctx, self, ir_inst) {
+        isle::lower(ctx, self, ir_inst)
-            return Ok(temp_regs);
-        }
-
-        let ty = if ctx.num_outputs(ir_inst) > 0 {
-            Some(ctx.output_ty(ir_inst, 0))
-        } else {
-            None
-        };
-
-        unreachable!(
-            "not implemented in ISLE: inst = `{}`, type = `{:?}`",
-            ctx.dfg().display_inst(ir_inst),
-            ty
-        );
     }
 
-    fn lower_branch_group(
+    fn lower_branch(
         &self,
         ctx: &mut Lower<Inst>,
-        branches: &[IRInst],
+        ir_inst: IRInst,
         targets: &[MachLabel],
-    ) -> CodegenResult<()> {
+    ) -> Option<()> {
-        // A block should end with at most two branches. The first may be a
+        isle::lower_branch(ctx, self, ir_inst, targets)
-        // conditional branch; a conditional branch can be followed only by an
-        // unconditional branch or fallthrough. Otherwise, if only one branch,
-        // it may be an unconditional branch, a fallthrough, a return, or a
-        // trap. These conditions are verified by `is_ebb_basic()` during the
-        // verifier pass.
-        assert!(branches.len() <= 2);
-        if branches.len() == 2 {
-            let op1 = ctx.data(branches[1]).opcode();
-            assert!(op1 == Opcode::Jump);
-        }
-
-        // Lower the first branch in ISLE.  This will automatically handle
-        // the second branch (if any) by emitting a two-way conditional branch.
-        if let Some(temp_regs) = super::lower::isle::lower_branch(ctx, self, branches[0], targets) {
-            assert!(temp_regs.len() == 0);
-            return Ok(());
-        }
-
-        unreachable!(
-            "not implemented in ISLE: branch = `{}`",
-            ctx.dfg().display_inst(branches[0]),
-        );
     }
 }

cranelift/codegen/src/isa/s390x/lower/isle.rs

@@ -71,7 +71,7 @@ pub(crate) fn lower_branch(
     backend: &S390xBackend,
     branch: Inst,
     targets: &[MachLabel],
-) -> Option<InstOutput> {
+) -> Option<()> {
     // TODO: reuse the ISLE context across lowerings so we can reuse its
     // internal heap allocations.
     let mut isle_ctx = IsleContext { lower_ctx, backend };

cranelift/codegen/src/isa/x64/lower.isle

@@ -7,7 +7,7 @@
 ;; A variant of the main lowering constructor term, used for branches.
 ;; The only difference is that it gets an extra argument holding a vector
 ;; of branch targets to be used.
-(decl partial lower_branch (Inst MachLabelSlice) InstOutput)
+(decl partial lower_branch (Inst MachLabelSlice) Unit)
 
 ;;;; Rules for `iconst` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
@@ -2865,55 +2865,55 @@
 ;; Rules for `jump` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (rule (lower_branch (jump _ _) (single_target target))
-      (side_effect (jmp_known target)))
+      (emit_side_effect (jmp_known target)))
 
 ;; Rules for `brz` and `brnz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (rule 2 (lower_branch (brz (icmp cc a b) _ _) (two_targets taken not_taken))
       (let ((cmp IcmpCondResult (invert_icmp_cond_result (emit_cmp cc a b))))
-        (side_effect (jmp_cond_icmp cmp taken not_taken))))
+        (emit_side_effect (jmp_cond_icmp cmp taken not_taken))))
 
 (rule 2 (lower_branch (brz (uextend (icmp cc a b)) _ _) (two_targets taken not_taken))
       (let ((cmp IcmpCondResult (invert_icmp_cond_result (emit_cmp cc a b))))
-        (side_effect (jmp_cond_icmp cmp taken not_taken))))
+        (emit_side_effect (jmp_cond_icmp cmp taken not_taken))))
 
 
 (rule 2 (lower_branch (brz (fcmp cc a b) _ _) (two_targets taken not_taken))
       (let ((cmp FcmpCondResult (emit_fcmp (floatcc_inverse cc) a b)))
-        (side_effect (jmp_cond_fcmp cmp taken not_taken))))
+        (emit_side_effect (jmp_cond_fcmp cmp taken not_taken))))
 
 (rule 2 (lower_branch (brz (uextend (fcmp cc a b)) _ _) (two_targets taken not_taken))
       (let ((cmp FcmpCondResult (emit_fcmp (floatcc_inverse cc) a b)))
-        (side_effect (jmp_cond_fcmp cmp taken not_taken))))
+        (emit_side_effect (jmp_cond_fcmp cmp taken not_taken))))
 
 (rule 1 (lower_branch (brz val @ (value_type $I128) _ _) (two_targets taken not_taken))
-      (side_effect (jmp_cond_icmp (cmp_zero_i128 (CC.NZ) val) taken not_taken)))
+      (emit_side_effect (jmp_cond_icmp (cmp_zero_i128 (CC.NZ) val) taken not_taken)))
 
 (rule 0 (lower_branch (brz val @ (value_type (ty_int_bool_or_ref)) _ _) (two_targets taken not_taken))
-      (side_effect
+      (emit_side_effect
         (with_flags_side_effect (cmp_zero_int_bool_ref val)
                                 (jmp_cond (CC.Z) taken not_taken))))
 
 
 (rule 2 (lower_branch (brnz (icmp cc a b) _ _) (two_targets taken not_taken))
-      (side_effect (jmp_cond_icmp (emit_cmp cc a b) taken not_taken)))
+      (emit_side_effect (jmp_cond_icmp (emit_cmp cc a b) taken not_taken)))
 
 (rule 2 (lower_branch (brnz (fcmp cc a b) _ _) (two_targets taken not_taken))
       (let ((cmp FcmpCondResult (emit_fcmp cc a b)))
-        (side_effect (jmp_cond_fcmp cmp taken not_taken))))
+        (emit_side_effect (jmp_cond_fcmp cmp taken not_taken))))
 
 (rule 2 (lower_branch (brnz (uextend (icmp cc a b)) _ _) (two_targets taken not_taken))
-      (side_effect (jmp_cond_icmp (emit_cmp cc a b) taken not_taken)))
+      (emit_side_effect (jmp_cond_icmp (emit_cmp cc a b) taken not_taken)))
 
 (rule 2 (lower_branch (brnz (uextend (fcmp cc a b)) _ _) (two_targets taken not_taken))
       (let ((cmp FcmpCondResult (emit_fcmp cc a b)))
-        (side_effect (jmp_cond_fcmp cmp taken not_taken))))
+        (emit_side_effect (jmp_cond_fcmp cmp taken not_taken))))
 
 (rule 1 (lower_branch (brnz val @ (value_type $I128) _ _) (two_targets taken not_taken))
-      (side_effect (jmp_cond_icmp (cmp_zero_i128 (CC.Z) val) taken not_taken)))
+      (emit_side_effect (jmp_cond_icmp (cmp_zero_i128 (CC.Z) val) taken not_taken)))
 
 (rule 0 (lower_branch (brnz val @ (value_type (ty_int_bool_or_ref)) _ _) (two_targets taken not_taken))
-      (side_effect
+      (emit_side_effect
         (with_flags_side_effect (cmp_zero_int_bool_ref val)
                                 (jmp_cond (CC.NZ) taken not_taken))))
 
@@ -2944,7 +2944,7 @@
 ;; Rules for `br_table` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (rule (lower_branch (br_table idx @ (value_type ty) _ _) (jump_table_targets default_target jt_targets))
-      (side_effect (jmp_table_seq ty idx default_target jt_targets)))
+      (emit_side_effect (jmp_table_seq ty idx default_target jt_targets)))
 
 ;; Rules for `select_spectre_guard` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 

cranelift/codegen/src/isa/x64/lower.rs

@@ -299,51 +299,17 @@ fn lower_to_amode(ctx: &mut Lower<Inst>, spec: InsnInput, offset: i32) -> Amode
 impl LowerBackend for X64Backend {
     type MInst = Inst;
 
-    fn lower(&self, ctx: &mut Lower<Inst>, ir_inst: IRInst) -> CodegenResult<InstOutput> {
+    fn lower(&self, ctx: &mut Lower<Inst>, ir_inst: IRInst) -> Option<InstOutput> {
-        if let Some(temp_regs) = isle::lower(ctx, self, ir_inst) {
+        isle::lower(ctx, self, ir_inst)
-            return Ok(temp_regs);
-        }
-
-        let ty = if ctx.num_outputs(ir_inst) > 0 {
-            Some(ctx.output_ty(ir_inst, 0))
-        } else {
-            None
-        };
-
-        unreachable!(
-            "not implemented in ISLE: inst = `{}`, type = `{:?}`",
-            ctx.dfg().display_inst(ir_inst),
-            ty
-        );
     }
 
-    fn lower_branch_group(
+    fn lower_branch(
         &self,
         ctx: &mut Lower<Inst>,
-        branches: &[IRInst],
+        ir_inst: IRInst,
         targets: &[MachLabel],
-    ) -> CodegenResult<()> {
+    ) -> Option<()> {
-        // A block should end with at most two branches. The first may be a
+        isle::lower_branch(ctx, self, ir_inst, targets)
-        // conditional branch; a conditional branch can be followed only by an
-        // unconditional branch or fallthrough. Otherwise, if only one branch,
-        // it may be an unconditional branch, a fallthrough, a return, or a
-        // trap. These conditions are verified by `is_ebb_basic()` during the
-        // verifier pass.
-        assert!(branches.len() <= 2);
-        if branches.len() == 2 {
-            let op1 = ctx.data(branches[1]).opcode();
-            assert!(op1 == Opcode::Jump);
-        }
-
-        if let Some(temp_regs) = isle::lower_branch(ctx, self, branches[0], targets) {
-            assert!(temp_regs.len() == 0);
-            return Ok(());
-        }
-
-        unreachable!(
-            "not implemented in ISLE: branch = `{}`",
-            ctx.dfg().display_inst(branches[0]),
-        );
     }
 
     fn maybe_pinned_reg(&self) -> Option<Reg> {

cranelift/codegen/src/isa/x64/lower/isle.rs

@@ -68,7 +68,7 @@ pub(crate) fn lower_branch(
     backend: &X64Backend,
     branch: Inst,
     targets: &[MachLabel],
-) -> Option<InstOutput> {
+) -> Option<()> {
     // TODO: reuse the ISLE context across lowerings so we can reuse its
     // internal heap allocations.
     let mut isle_ctx = IsleContext { lower_ctx, backend };

cranelift/codegen/src/machinst/isle.rs

@@ -140,7 +140,7 @@ macro_rules! isle_lower_prelude_methods {
                         _ => None,
                     };
                     if let Some(insn) = insn {
-                        if let Ok(regs) = self.backend.lower(self.lower_ctx, insn) {
+                        if let Some(regs) = self.backend.lower(self.lower_ctx, insn) {
                             assert!(regs.len() == 1);
                             return regs[0];
                         }

cranelift/codegen/src/machinst/lower.rs

@@ -122,18 +122,22 @@ pub trait LowerBackend {
     /// For a branch, this function should not generate the actual branch
     /// instruction. However, it must force any values it needs for the branch
     /// edge (block-param actuals) into registers, because the actual branch
-    /// generation (`lower_branch_group()`) happens *after* any possible merged
+    /// generation (`lower_branch()`) happens *after* any possible merged
     /// out-edge.
-    fn lower(&self, ctx: &mut Lower<Self::MInst>, inst: Inst) -> CodegenResult<InstOutput>;
+    ///
+    /// Returns `None` if no lowering for the instruction was found.
+    fn lower(&self, ctx: &mut Lower<Self::MInst>, inst: Inst) -> Option<InstOutput>;
 
     /// Lower a block-terminating group of branches (which together can be seen
     /// as one N-way branch), given a vcode MachLabel for each target.
-    fn lower_branch_group(
+    ///
+    /// Returns `None` if no lowering for the branch was found.
+    fn lower_branch(
         &self,
         ctx: &mut Lower<Self::MInst>,
-        insts: &[Inst],
+        inst: Inst,
         targets: &[MachLabel],
-    ) -> CodegenResult<()>;
+    ) -> Option<()>;
 
     /// A bit of a hack: give a fixed register that always holds the result of a
     /// `get_pinned_reg` instruction, if known.  This allows elision of moves
@@ -740,7 +744,18 @@ impl<'func, I: VCodeInst> Lower<'func, I> {
             // or any of its outputs its used.
             if has_side_effect || value_needed {
                 trace!("lowering: inst {}: {:?}", inst, self.f.dfg[inst]);
-                let temp_regs = backend.lower(self, inst)?;
+                let temp_regs = backend.lower(self, inst).unwrap_or_else(|| {
+                    let ty = if self.num_outputs(inst) > 0 {
+                        Some(self.output_ty(inst, 0))
+                    } else {
+                        None
+                    };
+                    panic!(
+                        "should be implemented in ISLE: inst = `{}`, type = `{:?}`",
+                        self.f.dfg.display_inst(inst),
+                        ty
+                    )
+                });
 
                 // The ISLE generated code emits its own registers to define the
                 // instruction's lowered values in. However, other instructions
@@ -895,10 +910,29 @@ impl<'func, I: VCodeInst> Lower<'func, I> {
             branches,
             targets,
         );
+        // A block should end with at most two branches. The first may be a
+        // conditional branch; a conditional branch can be followed only by an
+        // unconditional branch or fallthrough. Otherwise, if only one branch,
+        // it may be an unconditional branch, a fallthrough, a return, or a
+        // trap. These conditions are verified by `is_ebb_basic()` during the
+        // verifier pass.
+        assert!(branches.len() <= 2);
+        if branches.len() == 2 {
+            assert!(self.data(branches[1]).opcode() == Opcode::Jump);
+        }
         // When considering code-motion opportunities, consider the current
         // program point to be the first branch.
         self.cur_inst = Some(branches[0]);
-        backend.lower_branch_group(self, branches, targets)?;
+        // Lower the first branch in ISLE.  This will automatically handle
+        // the second branch (if any) by emitting a two-way conditional branch.
+        backend
+            .lower_branch(self, branches[0], targets)
+            .unwrap_or_else(|| {
+                panic!(
+                    "should be implemented in ISLE: branch = `{}`",
+                    self.f.dfg.display_inst(branches[0]),
+                )
+            });
         let loc = self.srcloc(branches[0]);
         self.finish_ir_inst(loc);
         // Add block param outputs for current block.

cranelift/codegen/src/prelude_lower.isle

@@ -282,20 +282,23 @@
                                  (inst2 MInst)
                                  (inst3 MInst))))
 
+;; Emit given side-effectful instruction.
+(decl emit_side_effect (SideEffectNoResult) Unit)
+(rule (emit_side_effect (SideEffectNoResult.Inst inst))
+      (emit inst))
+(rule (emit_side_effect (SideEffectNoResult.Inst2 inst1 inst2))
+      (let ((_ Unit (emit inst1)))
+        (emit inst2)))
+(rule (emit_side_effect (SideEffectNoResult.Inst3 inst1 inst2 inst3))
+      (let ((_ Unit (emit inst1))
+            (_ Unit (emit inst2)))
+        (emit inst3)))
+
 ;; Create an empty `InstOutput`, but do emit the given side-effectful
 ;; instruction.
 (decl side_effect (SideEffectNoResult) InstOutput)
-(rule (side_effect (SideEffectNoResult.Inst inst))
+(rule (side_effect inst)
-      (let ((_ Unit (emit inst)))
+      (let ((_ Unit (emit_side_effect inst)))
-        (output_none)))
-(rule (side_effect (SideEffectNoResult.Inst2 inst1 inst2))
-      (let ((_ Unit (emit inst1))
-            (_ Unit (emit inst2)))
-        (output_none)))
-(rule (side_effect (SideEffectNoResult.Inst3 inst1 inst2 inst3))
-      (let ((_ Unit (emit inst1))
-            (_ Unit (emit inst2))
-            (_ Unit (emit inst3)))
         (output_none)))
 
 (decl side_effect_concat (SideEffectNoResult SideEffectNoResult) SideEffectNoResult)