Address review comments.

This commit is contained in:
Chris Fallin
2021-08-31 17:42:50 -07:00
parent b19fa4857f
commit 6389071e09
5 changed files with 37 additions and 25 deletions

View File

@@ -550,7 +550,7 @@ impl<'a, F: Function> Checker<'a, F> {
self.handle_edits(block, out, &mut insert_idx, ProgPoint::before(inst)); self.handle_edits(block, out, &mut insert_idx, ProgPoint::before(inst));
// If this is a safepoint, then check the spillslots at this point. // If this is a safepoint, then check the spillslots at this point.
if self.f.is_safepoint(inst) { if self.f.requires_refs_on_stack(inst) {
let slots = safepoint_slots.remove(&inst).unwrap_or_else(|| vec![]); let slots = safepoint_slots.remove(&inst).unwrap_or_else(|| vec![]);
let checkinst = CheckerInst::Safepoint { inst, slots }; let checkinst = CheckerInst::Safepoint { inst, slots };

View File

@@ -122,7 +122,7 @@ impl Function for Func {
0 0
} }
fn is_safepoint(&self, insn: Inst) -> bool { fn requires_refs_on_stack(&self, insn: Inst) -> bool {
self.insts[insn.index()].is_safepoint self.insts[insn.index()].is_safepoint
} }
@@ -569,7 +569,7 @@ impl std::fmt::Debug for Func {
i, params, succs, preds i, params, succs, preds
)?; )?;
for inst in blockrange.iter() { for inst in blockrange.iter() {
if self.is_safepoint(inst) { if self.requires_refs_on_stack(inst) {
write!(f, " -- SAFEPOINT --\n")?; write!(f, " -- SAFEPOINT --\n")?;
} }
write!( write!(

View File

@@ -1011,7 +1011,7 @@ impl<'a, F: Function> Env<'a, F> {
} }
} }
if self.func.is_safepoint(inst) { if self.func.requires_refs_on_stack(inst) {
log::trace!("inst{} is safepoint", inst.index()); log::trace!("inst{} is safepoint", inst.index());
self.safepoints.push(inst); self.safepoints.push(inst);
for vreg in live.iter() { for vreg in live.iter() {

View File

@@ -860,7 +860,7 @@ impl<'a, F: Function> Env<'a, F> {
return; return;
} }
for inst in from.inst().index()..=to.inst().index() { for inst in from.inst().index()..=to.inst().index() {
if this.func.is_safepoint(Inst::new(inst)) { if this.func.requires_refs_on_stack(Inst::new(inst)) {
redundant_moves.clear(); redundant_moves.clear();
return; return;
} }

View File

@@ -856,18 +856,17 @@ pub trait Function {
/// to the sum of blockparam counts for all successor blocks. /// to the sum of blockparam counts for all successor blocks.
fn branch_blockparam_arg_offset(&self, block: Block, insn: Inst) -> usize; fn branch_blockparam_arg_offset(&self, block: Block, insn: Inst) -> usize;
/// Determine whether an instruction is a safepoint and requires a stackmap. /// Determine whether an instruction requires all reference-typed
/// values to be placed onto the stack. For these instructions,
/// stackmaps will be provided.
/// ///
/// Strictly speaking, these two parts (is a safepoint, requires a /// This is usually associated with the concept of a "safepoint",
/// stackmap) are orthogonal. An instruction could want to see a /// though strictly speaking, a safepoint could also support
/// stackmap of refs on the stack (without forcing them), or it /// reference-typed values in registers if there were a way to
/// could want all refs to be on the stack (without knowing where /// denote their locations and if this were acceptable to the
/// they are). Only the latter strictly follows from "is a /// client. Usually garbage-collector implementations want to see
/// safepoint". But in practice, both are true at the same time, /// roots on the stack, so we do that for now.
/// so we combine the two notions: for regalloc2, a "safepoint fn requires_refs_on_stack(&self, _: Inst) -> bool {
/// instruction" is one that both forces refs onto the stack, and
/// provides a stackmap indicating where they are.
fn is_safepoint(&self, _: Inst) -> bool {
false false
} }
@@ -883,15 +882,28 @@ pub trait Function {
fn inst_operands(&self, insn: Inst) -> &[Operand]; fn inst_operands(&self, insn: Inst) -> &[Operand];
/// Get the clobbers for an instruction; these are the registers /// Get the clobbers for an instruction; these are the registers
/// that the instruction is known to overwrite, separate from its /// that, after the instruction has executed, hold values that are
/// outputs described by its `Operand`s. This can be used to, for /// arbitrary, separately from the usual outputs to the
/// example, describe ABI-specified registers that are not /// instruction. It is invalid to read a register that has been
/// preserved by a call instruction, or fixed physical registers /// clobbered; the register allocator is free to assume that
/// written by an instruction but not used as a vreg output, or /// clobbered registers are filled with garbage and available for
/// fixed physical registers used as temps within an instruction /// reuse. It will avoid storing any value in a clobbered register
/// out of necessity. Every register written to by an instruction /// that must be live across the instruction.
/// must either be described by an Operand of kind `Def` or `Mod`, ///
/// or else must be a "clobber". /// Another way of seeing this is that a clobber is equivalent to
/// an "early def" of a fresh vreg that is not used anywhere else
/// in the program, with a fixed-register constraint that places
/// it in a given PReg chosen by the client prior to regalloc.
///
/// Every register written by an instruction must either
/// correspond to (be assigned to) an Operand of kind `Def` or
/// `Mod`, or else must be a "clobber".
///
/// This can be used to, for example, describe ABI-specified
/// registers that are not preserved by a call instruction, or
/// fixed physical registers written by an instruction but not
/// used as a vreg output, or fixed physical registers used as
/// temps within an instruction out of necessity.
fn inst_clobbers(&self, insn: Inst) -> &[PReg]; fn inst_clobbers(&self, insn: Inst) -> &[PReg];
/// Get the number of `VReg` in use in this function. /// Get the number of `VReg` in use in this function.