s390x: clean up lower.rs (#4355)

Now that lowering is fully done in ISLE, clean up some code remnants
in lower.rs.  In particular, move code to lower/isle.rs where
possible, and inline lower_insn_to_regs into its caller and simplify.

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

@@ -1,88 +1,41 @@
 //! Lowering rules for S390x.
 
-use crate::ir::condcodes::IntCC;
 use crate::ir::Inst as IRInst;
-use crate::ir::{MemFlags, Opcode};
+use crate::ir::Opcode;
-use crate::isa::s390x::abi::*;
+use crate::isa::s390x::inst::Inst;
-use crate::isa::s390x::inst::*;
-use crate::isa::s390x::settings as s390x_settings;
 use crate::isa::s390x::S390xBackend;
-use crate::machinst::lower::*;
+use crate::machinst::{InsnOutput, LowerBackend, LowerCtx, MachLabel};
-use crate::machinst::*;
-use crate::settings::Flags;
 use crate::CodegenResult;
 use smallvec::SmallVec;
 
 pub mod isle;
 
-//============================================================================
-// Lowering: force instruction input into a register
-
-/// Sign-extend the low `from_bits` bits of `value` to a full u64.
-fn sign_extend_to_u64(value: u64, from_bits: u8) -> u64 {
-    assert!(from_bits <= 64);
-    if from_bits >= 64 {
-        value
-    } else {
-        (((value << (64 - from_bits)) as i64) >> (64 - from_bits)) as u64
-    }
-}
-
 //=============================================================================
-// Lowering: comparisons
+// Lowering-backend trait implementation.
 
-/// Determines whether this condcode interprets inputs as signed or
+impl LowerBackend for S390xBackend {
-/// unsigned.  See the documentation for the `icmp` instruction in
+    type MInst = Inst;
-/// cranelift-codegen/meta/src/shared/instructions.rs for further insights
-/// into this.
-pub fn condcode_is_signed(cc: IntCC) -> bool {
-    match cc {
-        IntCC::Equal => false,
-        IntCC::NotEqual => false,
-        IntCC::SignedGreaterThanOrEqual => true,
-        IntCC::SignedGreaterThan => true,
-        IntCC::SignedLessThanOrEqual => true,
-        IntCC::SignedLessThan => true,
-        IntCC::UnsignedGreaterThanOrEqual => false,
-        IntCC::UnsignedGreaterThan => false,
-        IntCC::UnsignedLessThanOrEqual => false,
-        IntCC::UnsignedLessThan => false,
-        IntCC::Overflow => true,
-        IntCC::NotOverflow => true,
-    }
-}
 
-//============================================================================
+    fn lower<C: LowerCtx<I = Inst>>(&self, ctx: &mut C, ir_inst: IRInst) -> CodegenResult<()> {
-// Lowering: main entry point for lowering a instruction
+        let op = ctx.data(ir_inst).opcode();
-
+        let outputs: SmallVec<[InsnOutput; 2]> = (0..ctx.num_outputs(ir_inst))
-fn lower_insn_to_regs<C: LowerCtx<I = Inst>>(
+            .map(|i| InsnOutput {
-    ctx: &mut C,
+                insn: ir_inst,
-    insn: IRInst,
+                output: i,
-    flags: &Flags,
+            })
-    isa_flags: &s390x_settings::Flags,
-) -> CodegenResult<()> {
-    let op = ctx.data(insn).opcode();
-    let outputs: SmallVec<[InsnOutput; 2]> = (0..ctx.num_outputs(insn))
-        .map(|i| InsnOutput { insn, output: i })
             .collect();
         let ty = if outputs.len() > 0 {
-        Some(ctx.output_ty(insn, 0))
+            Some(ctx.output_ty(ir_inst, 0))
         } else {
             None
         };
 
-    if let Ok(()) = super::lower::isle::lower(ctx, flags, isa_flags, &outputs, insn) {
+        if let Ok(()) =
+            super::lower::isle::lower(ctx, &self.flags, &self.isa_flags, &outputs, ir_inst)
+        {
             return Ok(());
         }
 
-    let implemented_in_isle = || {
-        unreachable!(
-            "implemented in ISLE: inst = `{}`, type = `{:?}`",
-            ctx.dfg().display_inst(insn),
-            ty
-        );
-    };
-
         match op {
             Opcode::Nop
             | Opcode::Copy
@@ -186,42 +139,27 @@ fn lower_insn_to_regs<C: LowerCtx<I = Inst>>(
             | Opcode::Return
             | Opcode::StackAddr
             | Opcode::FuncAddr
-        | Opcode::SymbolValue => implemented_in_isle(),
+            | Opcode::SymbolValue => {
-
+                unreachable!(
-        Opcode::UaddSat | Opcode::SaddSat => unimplemented!(),
+                    "implemented in ISLE: inst = `{}`, type = `{:?}`",
-        Opcode::UsubSat | Opcode::SsubSat => unimplemented!(),
+                    ctx.dfg().display_inst(ir_inst),
-
+                    ty
-        Opcode::Bitrev => unimplemented!(),
+                )
-
-        Opcode::FcvtLowFromSint => unimplemented!("FcvtLowFromSint"),
-
-        Opcode::StackLoad | Opcode::StackStore => {
-            panic!("Direct stack memory access not supported; should not be used by Wasm");
             }
 
-        Opcode::ConstAddr => unimplemented!(),
+            Opcode::UaddSat
-
+            | Opcode::SaddSat
-        Opcode::HeapAddr => {
+            | Opcode::UsubSat
-            panic!("heap_addr should have been removed by legalization!");
+            | Opcode::SsubSat
-        }
+            | Opcode::Bitrev
-
+            | Opcode::FcvtLowFromSint
-        Opcode::TableAddr => {
+            | Opcode::ConstAddr
-            panic!("table_addr should have been removed by legalization!");
+            | Opcode::TlsValue
-        }
+            | Opcode::GetPinnedReg
-
+            | Opcode::SetPinnedReg
-        Opcode::GlobalValue => {
+            | Opcode::Isplit
-            panic!("global_value should have been removed by legalization!");
+            | Opcode::Iconcat
-        }
+            | Opcode::RawBitcast
-
-        Opcode::TlsValue => {
-            unimplemented!("Thread-local storage support not implemented!");
-        }
-
-        Opcode::GetPinnedReg | Opcode::SetPinnedReg => {
-            unimplemented!("Pinned register support not implemented!");
-        }
-
-        Opcode::RawBitcast
             | Opcode::Splat
             | Opcode::Swizzle
             | Opcode::Insertlane
@@ -260,17 +198,27 @@ fn lower_insn_to_regs<C: LowerCtx<I = Inst>>(
             | Opcode::FvpromoteLow
             | Opcode::Fvdemote
             | Opcode::IaddPairwise => {
-            // TODO
+                unreachable!(
-            unimplemented!("Vector ops not implemented.");
+                    "TODO: not yet implemented in ISLE: inst = `{}`, type = `{:?}`",
+                    ctx.dfg().display_inst(ir_inst),
+                    ty
+                )
             }
 
-        Opcode::Isplit | Opcode::Iconcat => unimplemented!("Wide integer ops not implemented."),
+            Opcode::StackLoad | Opcode::StackStore => {
-
+                panic!("Direct stack memory access not supported; should not be used by Wasm");
-        Opcode::IfcmpSp => {
+            }
-            panic!("Unused opcode should not be encountered.");
+            Opcode::HeapAddr => {
+                panic!("heap_addr should have been removed by legalization!");
+            }
+            Opcode::TableAddr => {
+                panic!("table_addr should have been removed by legalization!");
+            }
+            Opcode::GlobalValue => {
+                panic!("global_value should have been removed by legalization!");
             }
-
             Opcode::Ifcmp
+            | Opcode::IfcmpSp
             | Opcode::Ffcmp
             | Opcode::Trapff
             | Opcode::Trueif
@@ -278,7 +226,6 @@ fn lower_insn_to_regs<C: LowerCtx<I = Inst>>(
             | Opcode::Selectif => {
                 panic!("Flags opcode should not be encountered.");
             }
-
             Opcode::Jump
             | Opcode::Brz
             | Opcode::Brnz
@@ -288,7 +235,6 @@ fn lower_insn_to_regs<C: LowerCtx<I = Inst>>(
             | Opcode::BrTable => {
                 panic!("Branch opcode reached non-branch lowering logic!");
             }
-
             Opcode::IaddImm
             | Opcode::ImulImm
             | Opcode::UdivImm
@@ -320,18 +266,6 @@ fn lower_insn_to_regs<C: LowerCtx<I = Inst>>(
                 panic!("ALU+imm and ALU+carry ops should not appear here!");
             }
         }
-
-    Ok(())
-}
-
-//=============================================================================
-// Lowering-backend trait implementation.
-
-impl LowerBackend for S390xBackend {
-    type MInst = Inst;
-
-    fn lower<C: LowerCtx<I = Inst>>(&self, ctx: &mut C, ir_inst: IRInst) -> CodegenResult<()> {
-        lower_insn_to_regs(ctx, ir_inst, &self.flags, &self.isa_flags)
     }
 
     fn lower_branch_group<C: LowerCtx<I = Inst>>(

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

@@ -4,17 +4,19 @@
 pub mod generated_code;
 
 // Types that the generated ISLE code uses via `use super::*`.
-use super::{
+use crate::isa::s390x::abi::S390xMachineDeps;
-    CallIndInfo, CallInfo, Cond, Inst as MInst, MachLabel, MemArg, MemFlags, Opcode, Reg,
+use crate::isa::s390x::inst::{
-    S390xMachineDeps, UImm16Shifted, UImm32Shifted,
+    stack_reg, writable_gpr, zero_reg, CallIndInfo, CallInfo, Cond, Inst as MInst, MemArg,
+    UImm16Shifted, UImm32Shifted,
 };
 use crate::isa::s390x::settings::Flags as IsaFlags;
 use crate::machinst::isle::*;
+use crate::machinst::{MachLabel, Reg};
 use crate::settings::Flags;
 use crate::{
     ir::{
         condcodes::*, immediates::*, types::*, AtomicRmwOp, Endianness, Inst, InstructionData,
-        StackSlot, TrapCode, Value, ValueList,
+        MemFlags, Opcode, StackSlot, TrapCode, Value, ValueList,
     },
     isa::unwind::UnwindInst,
     machinst::{InsnOutput, LowerCtx, VCodeConstant, VCodeConstantData},
@@ -150,12 +152,12 @@ where
 
     #[inline]
     fn writable_gpr(&mut self, regno: u8) -> WritableReg {
-        super::writable_gpr(regno)
+        writable_gpr(regno)
     }
 
     #[inline]
     fn zero_reg(&mut self) -> Reg {
-        super::zero_reg()
+        zero_reg()
     }
 
     #[inline]
@@ -282,10 +284,7 @@ where
         let inst = self.lower_ctx.dfg().value_def(val).inst()?;
         let constant = self.lower_ctx.get_constant(inst)?;
         let ty = self.lower_ctx.output_ty(inst, 0);
-        Some(super::sign_extend_to_u64(
+        Some(sign_extend_to_u64(constant, self.ty_bits(ty).unwrap()))
-            constant,
-            self.ty_bits(ty).unwrap(),
-        ))
     }
 
     #[inline]
@@ -390,7 +389,7 @@ where
 
     #[inline]
     fn signed(&mut self, cc: &IntCC) -> Option<()> {
-        if super::condcode_is_signed(*cc) {
+        if condcode_is_signed(*cc) {
             Some(())
         } else {
             None
@@ -399,7 +398,7 @@ where
 
     #[inline]
     fn unsigned(&mut self, cc: &IntCC) -> Option<()> {
-        if !super::condcode_is_signed(*cc) {
+        if !condcode_is_signed(*cc) {
             Some(())
         } else {
             None
@@ -468,7 +467,7 @@ where
 
     #[inline]
     fn memarg_stack_off(&mut self, base: i64, off: i64) -> MemArg {
-        MemArg::reg_plus_off(super::stack_reg(), base + off, MemFlags::trusted())
+        MemArg::reg_plus_off(stack_reg(), base + off, MemFlags::trusted())
     }
 
     #[inline]
@@ -555,3 +554,36 @@ where
         self.lower_ctx.emit(inst.clone());
     }
 }
+
+/// Sign-extend the low `from_bits` bits of `value` to a full u64.
+#[inline]
+fn sign_extend_to_u64(value: u64, from_bits: u8) -> u64 {
+    assert!(from_bits <= 64);
+    if from_bits >= 64 {
+        value
+    } else {
+        (((value << (64 - from_bits)) as i64) >> (64 - from_bits)) as u64
+    }
+}
+
+/// Determines whether this condcode interprets inputs as signed or
+/// unsigned.  See the documentation for the `icmp` instruction in
+/// cranelift-codegen/meta/src/shared/instructions.rs for further insights
+/// into this.
+#[inline]
+fn condcode_is_signed(cc: IntCC) -> bool {
+    match cc {
+        IntCC::Equal => false,
+        IntCC::NotEqual => false,
+        IntCC::SignedGreaterThanOrEqual => true,
+        IntCC::SignedGreaterThan => true,
+        IntCC::SignedLessThanOrEqual => true,
+        IntCC::SignedLessThan => true,
+        IntCC::UnsignedGreaterThanOrEqual => false,
+        IntCC::UnsignedGreaterThan => false,
+        IntCC::UnsignedLessThanOrEqual => false,
+        IntCC::UnsignedLessThan => false,
+        IntCC::Overflow => true,
+        IntCC::NotOverflow => true,
+    }
+}