Skip to content

Commit

Permalink
Remove support for program moves (#118)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Amanieu
Amanieu authored Mar 5, 2023
1 parent 54f074e commit 7354cfe
Show file tree
Hide file tree
Showing 9 changed files with 4 additions and 415 deletions.
5 changes: 1 addition & 4 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@
This is a register allocator that started life as, and is about 50%
still, a port of IonMonkey's backtracking register allocator to
Rust. In many regards, it has been generalized, optimized, and
improved since the initial port, and now supports both SSA and non-SSA
use-cases. (However, non-SSA should be considered deprecated; we want to
move to SSA-only in the future, to enable some performance improvements.
See #4.)
improved since the initial port.

In addition, it contains substantial amounts of testing infrastructure
(fuzzing harnesses and checkers) that does not exist in the original
Expand Down
79 changes: 1 addition & 78 deletions src/checker.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,10 +269,6 @@ fn visit_all_vregs<F: Function, V: FnMut(VReg)>(f: &F, mut v: V) {
for op in f.inst_operands(inst) {
v(op.vreg());
}
if let Some((src, dst)) = f.is_move(inst) {
v(src.vreg());
v(dst.vreg());
}
if f.is_branch(inst) {
for succ_idx in 0..f.block_succs(block).len() {
for &param in f.branch_blockparams(block, inst, succ_idx) {
Expand Down Expand Up @@ -565,25 +561,6 @@ impl CheckerState {
// according to the move semantics in the step
// function below.
}
&CheckerInst::ProgramMove { inst, src, dst: _ } => {
// Validate that the fixed-reg constraint, if any, on
// `src` is satisfied.
if let OperandConstraint::FixedReg(preg) = src.constraint() {
let alloc = Allocation::reg(preg);
let val = self.get_value(&alloc).unwrap_or(&default_val);
trace!(
"checker: checkinst {:?}: cheker value in {:?} is {:?}",
checkinst,
alloc,
val
);
self.check_val(inst, src, alloc, val, &[alloc], checker)?;
}
// Note that we don't do anything with `dst`
// here. That is implicitly checked whenever `dst` is
// used; the `update()` step below adds the symbolic
// vreg for `dst` into wherever `src` may be stored.
}
}
Ok(())
}
Expand Down Expand Up @@ -686,15 +663,6 @@ impl CheckerState {
}
}
}
&CheckerInst::ProgramMove { inst: _, src, dst } => {
// Remove all earlier instances of `dst`: this vreg is
// now stale (it is being overwritten).
self.remove_vreg(dst.vreg());
// Define `dst` wherever `src` occurs.
for (_, value) in self.get_mappings_mut() {
value.copy_vreg(src.vreg(), dst.vreg());
}
}
}
}

Expand Down Expand Up @@ -786,23 +754,6 @@ pub(crate) enum CheckerInst {
/// A safepoint, with the given Allocations specified as containing
/// reftyped values. All other reftyped values become invalid.
Safepoint { inst: Inst, allocs: Vec<Allocation> },

/// An op with one source operand, and one dest operand, that
/// copies any symbolic values from the source to the dest, in
/// addition to adding the symbolic value of the dest vreg to the
/// set. This "program move" is distinguished from the above
/// `Move` by being semantically relevant in the original
/// (pre-regalloc) program.
///
/// We transform checker values as follows: for any vreg-set that
/// contains `dst`'s vreg, we first delete that vreg (because it
/// is being redefined). Then, for any vreg-set with `src`
/// present, we add `dst`.
ProgramMove {
inst: Inst,
src: Operand,
dst: Operand,
},
}

#[derive(Debug)]
Expand Down Expand Up @@ -903,35 +854,10 @@ impl<'a, F: Function> Checker<'a, F> {
self.bb_insts.get_mut(&block).unwrap().push(checkinst);
}

// If this is a move, handle specially. Note that the
// regalloc2-inserted moves are not semantically present in
// the original program and so do not modify the sets of
// symbolic values at all, but rather just move them around;
// but "program moves" *are* present, and have the following
// semantics: they define the destination vreg, but also
// retain any symbolic values in the source.
//
// regalloc2 reifies all moves into edits in its unified
// move/edit framework, so we don't get allocs for these moves
// in the post-regalloc output, and the embedder is not
// supposed to emit the moves. But we *do* want to check the
// semantic implications, namely definition of new vregs. So
// we emit `ProgramMove` ops that do just this.
if let Some((src, dst)) = self.f.is_move(inst) {
let src_op = Operand::any_use(src.vreg());
let dst_op = Operand::any_def(dst.vreg());
let checkinst = CheckerInst::ProgramMove {
inst,
src: src_op,
dst: dst_op,
};
trace!("checker: adding inst {:?}", checkinst);
self.bb_insts.get_mut(&block).unwrap().push(checkinst);
}
// Skip normal checks if this is a branch: the blockparams do
// not exist in post-regalloc code, and the edge-moves have to
// be inserted before the branch rather than after.
else if !self.f.is_branch(inst) {
if !self.f.is_branch(inst) {
let operands: Vec<_> = self.f.inst_operands(inst).iter().cloned().collect();
let allocs: Vec<_> = out.inst_allocs(inst).iter().cloned().collect();
let clobbers: Vec<_> = self.f.inst_clobbers(inst).into_iter().collect();
Expand Down Expand Up @@ -1128,9 +1054,6 @@ impl<'a, F: Function> Checker<'a, F> {
}
trace!(" safepoint: {}", slotargs.join(", "));
}
&CheckerInst::ProgramMove { inst, src, dst } => {
trace!(" inst{}: prog_move {} -> {}", inst.index(), src, dst);
}
&CheckerInst::ParallelMove { .. } => {
panic!("unexpected parallel_move in body (non-edge)")
}
Expand Down
4 changes: 0 additions & 4 deletions src/fuzzing/func.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,6 @@ impl Function for Func {
&self.debug_value_labels[..]
}

fn is_move(&self, _: Inst) -> Option<(Operand, Operand)> {
None
}

fn inst_operands(&self, insn: Inst) -> &[Operand] {
&self.insts[insn.index()].operands[..]
}
Expand Down
21 changes: 0 additions & 21 deletions src/ion/data_structures.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,21 +405,6 @@ pub struct Env<'a, F: Function> {
pub extra_spillslots_by_class: [SmallVec<[Allocation; 2]>; 2],
pub preferred_victim_by_class: [PReg; 2],

// Program moves: these are moves in the provided program that we
// handle with our internal machinery, in order to avoid the
// overhead of ordinary operand processing. We expect the client
// to not generate any code for instructions that return
// `Some(..)` for `.is_move()`, and instead use the edits that we
// provide to implement those moves (or some simplified version of
// them) post-regalloc.
//
// (from-vreg, inst, from-alloc), sorted by (from-vreg, inst)
pub prog_move_srcs: Vec<((VRegIndex, Inst), Allocation)>,
// (to-vreg, inst, to-alloc), sorted by (to-vreg, inst)
pub prog_move_dsts: Vec<((VRegIndex, Inst), Allocation)>,
// (from-vreg, to-vreg) for bundle-merging.
pub prog_move_merges: Vec<(LiveRangeIndex, LiveRangeIndex)>,

// When multiple fixed-register constraints are present on a
// single VReg at a single program point (this can happen for,
// e.g., call args that use the same value multiple times), we
Expand Down Expand Up @@ -628,9 +613,7 @@ pub struct InsertedMove {
#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub enum InsertMovePrio {
InEdgeMoves,
BlockParam,
Regular,
PostRegular,
MultiFixedRegInitial,
MultiFixedRegSecondary,
ReusedInput,
Expand Down Expand Up @@ -660,10 +643,6 @@ pub struct Stats {
pub livein_iterations: usize,
pub initial_liverange_count: usize,
pub merged_bundle_count: usize,
pub prog_moves: usize,
pub prog_moves_dead_src: usize,
pub prog_move_merge_attempt: usize,
pub prog_move_merge_success: usize,
pub process_bundle_count: usize,
pub process_bundle_reg_probes_fixed: usize,
pub process_bundle_reg_success_fixed: usize,
Expand Down
154 changes: 0 additions & 154 deletions src/ion/liveranges.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,13 +356,6 @@ impl<'a, F: Function> Env<'a, F> {
}

for inst in insns.rev().iter() {
if let Some((src, dst)) = self.func.is_move(inst) {
live.set(dst.vreg().vreg(), false);
live.set(src.vreg().vreg(), true);
self.observe_vreg_class(src.vreg());
self.observe_vreg_class(dst.vreg());
}

for pos in &[OperandPos::Late, OperandPos::Early] {
for op in self.func.inst_operands(inst) {
if op.as_fixed_nonallocatable().is_some() {
Expand Down Expand Up @@ -519,148 +512,6 @@ impl<'a, F: Function> Env<'a, F> {
}
}

// If this is a move, handle specially.
if let Some((src, dst)) = self.func.is_move(inst) {
assert!(
src.vreg() != dst.vreg(),
"Invalid move: overwriting an SSA value"
);

trace!(" -> move inst{}: src {} -> dst {}", inst.index(), src, dst);

debug_assert_eq!(src.class(), dst.class());
debug_assert_eq!(src.kind(), OperandKind::Use);
debug_assert_eq!(src.pos(), OperandPos::Early);
debug_assert_eq!(dst.kind(), OperandKind::Def);
debug_assert_eq!(dst.pos(), OperandPos::Late);

// Redefine src and dst operands to have
// positions of After and Before respectively
// (see note below), and to have Any
// constraints if they were originally Reg.
let src_constraint = match src.constraint() {
OperandConstraint::Reg => OperandConstraint::Any,
x => x,
};
let dst_constraint = match dst.constraint() {
OperandConstraint::Reg => OperandConstraint::Any,
x => x,
};
let src = Operand::new(
src.vreg(),
src_constraint,
OperandKind::Use,
OperandPos::Late,
);
let dst = Operand::new(
dst.vreg(),
dst_constraint,
OperandKind::Def,
OperandPos::Early,
);

if self.annotations_enabled {
self.annotate(
ProgPoint::after(inst),
format!(
" prog-move v{} ({:?}) -> v{} ({:?})",
src.vreg().vreg(),
src_constraint,
dst.vreg().vreg(),
dst_constraint,
),
);
}

// N.B.: in order to integrate with the move
// resolution that joins LRs in general, we
// conceptually treat the move as happening
// between the move inst's After and the next
// inst's Before. Thus the src LR goes up to
// (exclusive) next-inst-pre, and the dst LR
// starts at next-inst-pre. We have to take
// care in our move insertion to handle this
// like other inter-inst moves, i.e., at
// `Regular` priority, so it properly happens
// in parallel with other inter-LR moves.
//
// Why the progpoint between move and next
// inst, and not the progpoint between prev
// inst and move? Because a move can be the
// first inst in a block, but cannot be the
// last; so the following progpoint is always
// within the same block, while the previous
// one may be an inter-block point (and the
// After of the prev inst in a different
// block).

// Handle the def w.r.t. liveranges: trim the
// start of the range and mark it dead at this
// point in our backward scan.
let pos = ProgPoint::before(inst.next());
let mut dst_lr = vreg_ranges[dst.vreg().vreg()];
if !live.get(dst.vreg().vreg()) {
let from = pos;
let to = pos.next();
dst_lr = self.add_liverange_to_vreg(
VRegIndex::new(dst.vreg().vreg()),
CodeRange { from, to },
);
trace!(" -> invalid LR for def; created {:?}", dst_lr);
}
trace!(" -> has existing LR {:?}", dst_lr);
// Trim the LR to start here.
if self.ranges[dst_lr.index()].range.from
== self.cfginfo.block_entry[block.index()]
{
trace!(" -> started at block start; trimming to {:?}", pos);
self.ranges[dst_lr.index()].range.from = pos;
}
self.ranges[dst_lr.index()].set_flag(LiveRangeFlag::StartsAtDef);
live.set(dst.vreg().vreg(), false);
vreg_ranges[dst.vreg().vreg()] = LiveRangeIndex::invalid();

// Handle the use w.r.t. liveranges: make it live
// and create an initial LR back to the start of
// the block.
let pos = ProgPoint::after(inst);
let src_lr = if !live.get(src.vreg().vreg()) {
let range = CodeRange {
from: self.cfginfo.block_entry[block.index()],
to: pos.next(),
};
let src_lr =
self.add_liverange_to_vreg(VRegIndex::new(src.vreg().vreg()), range);
vreg_ranges[src.vreg().vreg()] = src_lr;
src_lr
} else {
vreg_ranges[src.vreg().vreg()]
};

trace!(" -> src LR {:?}", src_lr);

// Add to live-set.
let src_is_dead_after_move = !live.get(src.vreg().vreg());
live.set(src.vreg().vreg(), true);

// Add to program-moves lists.
self.prog_move_srcs.push((
(VRegIndex::new(src.vreg().vreg()), inst),
Allocation::none(),
));
self.prog_move_dsts.push((
(VRegIndex::new(dst.vreg().vreg()), inst.next()),
Allocation::none(),
));
self.stats.prog_moves += 1;
if src_is_dead_after_move {
self.stats.prog_moves_dead_src += 1;
self.prog_move_merges.push((src_lr, dst_lr));
}

continue;
}

// Preprocess defs and uses. Specifically, if there
// are any fixed-reg-constrained defs at Late position
// and fixed-reg-constrained uses at Early position
Expand Down Expand Up @@ -1016,11 +867,6 @@ impl<'a, F: Function> Env<'a, F> {

self.blockparam_ins.sort_unstable_by_key(|x| x.key());
self.blockparam_outs.sort_unstable_by_key(|x| x.key());
self.prog_move_srcs.sort_unstable_by_key(|(pos, _)| *pos);
self.prog_move_dsts.sort_unstable_by_key(|(pos, _)| *pos);

trace!("prog_move_srcs = {:?}", self.prog_move_srcs);
trace!("prog_move_dsts = {:?}", self.prog_move_dsts);

self.stats.initial_liverange_count = self.ranges.len();
self.stats.blockparam_ins_count = self.blockparam_ins.len();
Expand Down
22 changes: 0 additions & 22 deletions src/ion/merge.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -351,28 +351,6 @@ impl<'a, F: Function> Env<'a, F> {
self.merge_bundles(from_bundle, to_bundle);
}

// Attempt to merge move srcs/dsts.
for i in 0..self.prog_move_merges.len() {
let (src, dst) = self.prog_move_merges[i];
trace!("trying to merge move src LR {:?} to dst LR {:?}", src, dst);
let src = self.resolve_merged_lr(src);
let dst = self.resolve_merged_lr(dst);
trace!(
"resolved LR-construction merging chains: move-merge is now src LR {:?} to dst LR {:?}",
src,
dst
);

let src_bundle = self.ranges[src.index()].bundle;
debug_assert!(src_bundle.is_valid());
let dest_bundle = self.ranges[dst.index()].bundle;
debug_assert!(dest_bundle.is_valid());
self.stats.prog_move_merge_attempt += 1;
if self.merge_bundles(/* from */ dest_bundle, /* to */ src_bundle) {
self.stats.prog_move_merge_success += 1;
}
}

trace!("done merging bundles");
}

Expand Down
Loading

0 comments on commit 7354cfe

Please sign in to comment.