Address review comments.

Author: cfallin

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

@@ -404,7 +404,8 @@ fn in_int_reg(ty: ir::Type) -> bool {
     match ty {
         types::I8 | types::I16 | types::I32 | types::I64 => true,
         types::B1 | types::B8 | types::B16 | types::B32 | types::B64 => true,
-        types::R32 | types::R64 => true,
+        types::R64 => true,
+        types::R32 => panic!("Unexpected 32-bit reference on a 64-bit platform!"),
         _ => false,
     }
 }
@@ -1134,7 +1135,8 @@ impl ABIBody for AArch64ABIBody {
         }
 
         // N.B.: "nominal SP", which we use to refer to stackslots and
-        // spillslots, is right here.
+        // spillslots, is defined to be equal to the stack pointer at this point
+        // in the prologue.
         //
         // If we push any clobbers below, we emit a virtual-SP adjustment
         // meta-instruction so that the nominal-SP references behave as if SP
@@ -1322,9 +1324,21 @@ impl ABIBody for AArch64ABIBody {
     }
 }
 
+/// Return a type either from an optional type hint, or if not, from the default
+/// type associated with the given register's class. This is used to generate
+/// loads/spills appropriately given the type of value loaded/stored (which may
+/// be narrower than the spillslot). We usually have the type because the
+/// regalloc usually provides the vreg being spilled/reloaded, and we know every
+/// vreg's type. However, the regalloc *can* request a spill/reload without an
+/// associated vreg when needed to satisfy a safepoint (which requires all
+/// ref-typed values, even those in real registers in the original vcode, to be
+/// in spillslots).
 fn ty_from_ty_hint_or_reg_class(r: Reg, ty: Option<Type>) -> Type {
     match (ty, r.get_class()) {
+        // If the type is provided
         (Some(t), _) => t,
+        // If no type is provided, this should be a register spill for a
+        // safepoint, so we only expect I64 (integer) registers.
         (None, RegClass::I64) => I64,
         _ => panic!("Unexpected register class!"),
     }

cranelift/codegen/src/isa/aarch64/inst/mod.rs

@@ -2138,7 +2138,7 @@ impl MachInst for Inst {
         44
     }
 
-    fn ref_type_rc(_: &settings::Flags) -> RegClass {
+    fn ref_type_regclass(_: &settings::Flags) -> RegClass {
         RegClass::I64
     }
 }

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

@@ -588,9 +588,21 @@ impl ABIBody for X64ABIBody {
     }
 }
 
+/// Return a type either from an optional type hint, or if not, from the default
+/// type associated with the given register's class. This is used to generate
+/// loads/spills appropriately given the type of value loaded/stored (which may
+/// be narrower than the spillslot). We usually have the type because the
+/// regalloc usually provides the vreg being spilled/reloaded, and we know every
+/// vreg's type. However, the regalloc *can* request a spill/reload without an
+/// associated vreg when needed to satisfy a safepoint (which requires all
+/// ref-typed values, even those in real registers in the original vcode, to be
+/// in spillslots).
 fn ty_from_ty_hint_or_reg_class(r: Reg, ty: Option<Type>) -> Type {
     match (ty, r.get_class()) {
+        // If the type is provided
         (Some(t), _) => t,
+        // If no type is provided, this should be a register spill for a
+        // safepoint, so we only expect I64 (integer) registers.
         (None, RegClass::I64) => I64,
         _ => panic!("Unexpected register class!"),
     }

cranelift/codegen/src/isa/x64/inst/mod.rs

@@ -1258,7 +1258,7 @@ impl MachInst for Inst {
         15
     }
 
-    fn ref_type_rc(_: &settings::Flags) -> RegClass {
+    fn ref_type_regclass(_: &settings::Flags) -> RegClass {
         RegClass::I64
     }
 

cranelift/codegen/src/machinst/buffer.rs

@@ -1318,8 +1318,9 @@ pub struct MachSrcLoc {
 pub struct MachStackMap {
     /// The code offset at which this stackmap applies.
     pub offset: CodeOffset,
-    /// The code offset at the *end* of the instruction at which this stackmap
-    /// applies.
+    /// The code offset just past the "end" of the instruction: that is, the
+    /// offset of the first byte of the following instruction, or equivalently,
+    /// the start offset plus the instruction length.
     pub offset_end: CodeOffset,
     /// The Stackmap itself.
     pub stackmap: Stackmap,

cranelift/codegen/src/machinst/lower.rs

@@ -4,7 +4,6 @@
 
 use crate::entity::SecondaryMap;
 use crate::fx::{FxHashMap, FxHashSet};
-use crate::inst_predicates::is_safepoint;
 use crate::inst_predicates::{has_side_effect_or_load, is_constant_64bit};
 use crate::ir::instructions::BranchInfo;
 use crate::ir::types::I64;
@@ -94,8 +93,6 @@ pub trait LowerCtx {
     /// every side-effecting op; the backend should not try to merge across
     /// side-effect colors unless the op being merged is known to be pure.
     fn inst_color(&self, ir_inst: Inst) -> InstColor;
-    /// Determine whether an instruction is a safepoint.
-    fn is_safepoint(&self, ir_inst: Inst) -> bool;
 
     // Instruction input/output queries:
 
@@ -899,13 +896,6 @@ impl<'func, I: VCodeInst> LowerCtx for Lower<'func, I> {
         self.inst_colors[ir_inst]
     }
 
-    fn is_safepoint(&self, ir_inst: Inst) -> bool {
-        // There is no safepoint metadata at all if we have no reftyped values
-        // in this function; lack of metadata implies "nothing to trace", and
-        // avoids overhead.
-        self.vcode.have_ref_values() && is_safepoint(self.f, ir_inst)
-    }
-
     fn num_inputs(&self, ir_inst: Inst) -> usize {
         self.f.dfg.inst_args(ir_inst).len()
     }

cranelift/codegen/src/machinst/mod.rs

@@ -193,7 +193,7 @@ pub trait MachInst: Clone + Debug {
 
     /// What is the register class used for reference types (GC-observable pointers)? Can
     /// be dependent on compilation flags.
-    fn ref_type_rc(_flags: &Flags) -> RegClass;
+    fn ref_type_regclass(_flags: &Flags) -> RegClass;
 
     /// A label-use kind: a type that describes the types of label references that
     /// can occur in an instruction.

cranelift/codegen/src/machinst/vcode.rs

@@ -132,7 +132,7 @@ pub struct VCodeBuilder<I: VCodeInst> {
 impl<I: VCodeInst> VCodeBuilder<I> {
     /// Create a new VCodeBuilder.
     pub fn new(abi: Box<dyn ABIBody<I = I>>, block_order: BlockLoweringOrder) -> VCodeBuilder<I> {
-        let reftype_class = I::ref_type_rc(abi.flags());
+        let reftype_class = I::ref_type_regclass(abi.flags());
         let vcode = VCode::new(abi, block_order);
         let stackmap_info = StackmapRequestInfo {
             reftype_class,
@@ -257,7 +257,7 @@ fn is_redundant_move<I: VCodeInst>(insn: &I) -> bool {
 
 /// Is this type a reference type?
 fn is_reftype(ty: Type) -> bool {
-    ty == types::R32 || ty == types::R64
+    ty == types::R64 || ty == types::R32
 }
 
 impl<I: VCodeInst> VCode<I> {

cranelift/filetests/filetests/vcode/aarch64/reftypes.clif

@@ -12,18 +12,7 @@ block0(v0: r64):
 ; nextln: ldp fp, lr, [sp], #16
 ; nextln: ret
 
-function %f1(r32) -> r32 {
-block0(v0: r32):
-  return v0
-}
-
-; check: stp fp, lr, [sp, #-16]!
-; nextln: mov fp, sp
-; nextln: mov sp, fp
-; nextln: ldp fp, lr, [sp], #16
-; nextln: ret
-
-function %f2(r64) -> b1 {
+function %f1(r64) -> b1 {
 block0(v0: r64):
   v1 = is_null v0
   return v1
@@ -37,7 +26,7 @@ block0(v0: r64):
 ; nextln: ldp fp, lr, [sp], #16
 ; nextln: ret
 
-function %f3(r64) -> b1 {
+function %f2(r64) -> b1 {
 block0(v0: r64):
   v1 = is_invalid v0
   return v1
@@ -51,7 +40,7 @@ block0(v0: r64):
 ; nextln: ldp fp, lr, [sp], #16
 ; nextln: ret
 
-function %f4() -> r64 {
+function %f3() -> r64 {
 block0:
   v0 = null.r64
   return v0
@@ -64,7 +53,7 @@ block0:
 ; nextln: ldp fp, lr, [sp], #16
 ; nextln: ret
 
-function %f5(r64, r64) -> r64, r64, r64 {
+function %f4(r64, r64) -> r64, r64, r64 {
     fn0 = %f(r64) -> b1
     ss0 = explicit_slot 8