Address review comments.

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