Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: use &'static [PReg] instead of Vec<PReg> #208

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions regalloc2-tool/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ fn main() {
validate_ssa: true,
algorithm: args.algorithm.into(),
};
let output = match regalloc2::run(&function, function.machine_env(), &options) {
let output = match regalloc2::run(&function, &function.machine_env(), &options) {
Ok(output) => output,
Err(e) => {
panic!("Register allocation failed: {e:#?}");
Expand All @@ -63,7 +63,8 @@ fn main() {
print_output(&function, &output);
}

let mut checker = Checker::new(&function, function.machine_env());
let machine_env = function.machine_env();
let mut checker = Checker::new(&function, &machine_env);
checker.prepare(&output);
if let Err(e) = checker.run() {
panic!("Regsiter allocation checker failed: {e:#?}");
Expand Down
16 changes: 8 additions & 8 deletions src/checker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ impl CheckerState {
alloc: Allocation,
val: &CheckerValue,
allocs: &[Allocation],
checker: &Checker<'a, F>,
checker: &Checker<'a, '_, F>,
) -> Result<(), CheckerError> {
if alloc == Allocation::none() {
return Err(CheckerError::MissingAllocation { inst, op });
Expand Down Expand Up @@ -465,7 +465,7 @@ impl CheckerState {
&self,
pos: InstPosition,
checkinst: &CheckerInst,
checker: &Checker<'a, F>,
checker: &Checker<'a, '_, F>,
) -> Result<(), CheckerError> {
let default_val = Default::default();
match checkinst {
Expand Down Expand Up @@ -627,7 +627,7 @@ impl CheckerState {
op: Operand,
alloc: Allocation,
allocs: &[Allocation],
checker: &Checker<'a, F>,
checker: &Checker<'a, '_, F>,
) -> Result<(), CheckerError> {
match op.constraint() {
OperandConstraint::Any => {}
Expand Down Expand Up @@ -691,21 +691,21 @@ pub(crate) enum CheckerInst {
}

#[derive(Debug)]
pub struct Checker<'a, F: Function> {
pub struct Checker<'a, 'r, F: Function> {
f: &'a F,
bb_in: FxHashMap<Block, CheckerState>,
bb_insts: FxHashMap<Block, Vec<CheckerInst>>,
edge_insts: FxHashMap<(Block, Block), Vec<CheckerInst>>,
machine_env: &'a MachineEnv,
machine_env: &'a MachineEnv<'r>,
stack_pregs: PRegSet,
}

impl<'a, F: Function> Checker<'a, F> {
impl<'a, 'r, F: Function> Checker<'a, 'r, F> {
/// Create a new checker for the given function, initializing CFG
/// info immediately. The client should call the `add_*()`
/// methods to add abstract instructions to each BB before
/// invoking `run()` to check for errors.
pub fn new(f: &'a F, machine_env: &'a MachineEnv) -> Checker<'a, F> {
pub fn new(f: &'a F, machine_env: &'a MachineEnv<'r>) -> Checker<'a, 'r, F> {
let mut bb_in = FxHashMap::default();
let mut bb_insts = FxHashMap::default();
let mut edge_insts = FxHashMap::default();
Expand All @@ -722,7 +722,7 @@ impl<'a, F: Function> Checker<'a, F> {
bb_in.insert(f.entry_block(), CheckerState::default());

let mut stack_pregs = PRegSet::empty();
for &preg in &machine_env.fixed_stack_slots {
for &preg in machine_env.fixed_stack_slots {
stack_pregs.add(preg);
}

Expand Down
6 changes: 3 additions & 3 deletions src/fastalloc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,9 @@ impl<'a, F: Function> Env<'a, F> {
fn new(func: &'a F, env: &'a MachineEnv) -> Self {
use alloc::vec;
let mut regs = [
env.preferred_regs_by_class[RegClass::Int as usize].clone(),
env.preferred_regs_by_class[RegClass::Float as usize].clone(),
env.preferred_regs_by_class[RegClass::Vector as usize].clone(),
Vec::from(env.preferred_regs_by_class[RegClass::Int as usize]),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't love this, open for better ideas

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use .to_owned() instead of Vec::from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, yeah doesn't really change anything about it but might look nicer

Vec::from(env.preferred_regs_by_class[RegClass::Float as usize]),
Vec::from(env.preferred_regs_by_class[RegClass::Vector as usize]),
];
regs[0].extend(
env.non_preferred_regs_by_class[RegClass::Int as usize]
Expand Down
18 changes: 10 additions & 8 deletions src/fastalloc/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,18 +140,20 @@ impl RealFunction {
}
}

fn mach_env(no_of_regs: usize) -> MachineEnv {
fn mach_env(no_of_regs: usize) -> MachineEnv<'static> {
MachineEnv {
preferred_regs_by_class: [
(0..no_of_regs)
.map(|no| PReg::new(no, RegClass::Int))
.collect(),
vec![],
vec![],
Vec::leak(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely can't leak while fuzzing -- can we instead have a few static MachineEnvs in scope, and pick one of them? We don't necessarily need to fuzz with arbitrary no_of_regs, just with a few values that are interesting -- the two extremes of its range and a few in the middle, for example.

(0..no_of_regs)
.map(|no| PReg::new(no, RegClass::Int))
.collect(),
),
&[],
&[],
],
non_preferred_regs_by_class: [vec![], vec![], vec![]],
non_preferred_regs_by_class: [&[], &[], &[]],
scratch_by_class: [None, None, None],
fixed_stack_slots: vec![],
fixed_stack_slots: &[],
}
}

Expand Down
30 changes: 16 additions & 14 deletions src/fuzzing/func.rs
Original file line number Diff line number Diff line change
Expand Up @@ -624,30 +624,32 @@ impl core::fmt::Debug for Func {
}
}

pub fn machine_env() -> MachineEnv {
fn regs(r: core::ops::Range<usize>, c: RegClass) -> Vec<PReg> {
r.map(|i| PReg::new(i, c)).collect()
pub fn machine_env() -> MachineEnv<'static> {
fn regs(r: core::ops::Range<usize>, c: RegClass) -> &'static [PReg] {
Vec::leak(r.map(|i| PReg::new(i, c)).collect())
}
let preferred_regs_by_class: [Vec<PReg>; 3] = [
let preferred_regs_by_class: [&'static [PReg]; 3] = [
regs(0..24, RegClass::Int),
regs(0..24, RegClass::Float),
regs(0..24, RegClass::Vector),
];
let non_preferred_regs_by_class: [Vec<PReg>; 3] = [
let non_preferred_regs_by_class: [&'static [PReg]; 3] = [
regs(24..32, RegClass::Int),
regs(24..32, RegClass::Float),
regs(24..32, RegClass::Vector),
];
let scratch_by_class: [Option<PReg>; 3] = [None, None, None];
let fixed_stack_slots = (32..63)
.flat_map(|i| {
[
PReg::new(i, RegClass::Int),
PReg::new(i, RegClass::Float),
PReg::new(i, RegClass::Vector),
]
})
.collect();
let fixed_stack_slots = Vec::leak(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to avoid Vec::leak here: fuzzing will continually generate new functions with machine envs, and we cannot allocate indefinitely. Can you build a static initializer, since the range (32..63) is static?

(32..63)
.flat_map(|i| {
[
PReg::new(i, RegClass::Int),
PReg::new(i, RegClass::Float),
PReg::new(i, RegClass::Vector),
]
})
.collect(),
);
// Register 63 is reserved for use as a fixed non-allocatable register.
MachineEnv {
preferred_regs_by_class,
Expand Down
10 changes: 5 additions & 5 deletions src/ion/data_structures.rs
Original file line number Diff line number Diff line change
Expand Up @@ -497,27 +497,27 @@ impl Ctx {
}
}

pub struct Env<'a, F: Function> {
pub struct Env<'a, 'r, F: Function> {
pub func: &'a F,
pub env: &'a MachineEnv,
pub env: &'a MachineEnv<'r>,
pub ctx: &'a mut Ctx,
}

impl<'a, F: Function> Deref for Env<'a, F> {
impl<'a, F: Function> Deref for Env<'a, '_, F> {
type Target = Ctx;

fn deref(&self) -> &Self::Target {
self.ctx
}
}

impl<'a, F: Function> DerefMut for Env<'a, F> {
impl<'a, F: Function> DerefMut for Env<'a, '_, F> {
fn deref_mut(&mut self) -> &mut Self::Target {
self.ctx
}
}

impl<'a, F: Function> Env<'a, F> {
impl<'a, F: Function> Env<'a, '_, F> {
/// Get the VReg (with bundled RegClass) from a vreg index.
#[inline]
pub fn vreg(&self, index: VRegIndex) -> VReg {
Expand Down
2 changes: 1 addition & 1 deletion src/ion/dump.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use alloc::{string::String, vec::Vec};
use super::Env;
use crate::{Block, Function, ProgPoint};

impl<'a, F: Function> Env<'a, F> {
impl<'a, F: Function> Env<'a, '_, F> {
pub fn dump_state(&self) {
trace!("Bundles:");
for (i, b) in self.bundles.iter().enumerate() {
Expand Down
4 changes: 2 additions & 2 deletions src/ion/liveranges.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ impl core::ops::Add<SpillWeight> for SpillWeight {
}
}

impl<'a, F: Function> Env<'a, F> {
impl<'a, F: Function> Env<'a, '_, F> {
pub fn create_pregs_and_vregs(&mut self) {
// Create PRegs from the env.
self.pregs.resize(
Expand All @@ -104,7 +104,7 @@ impl<'a, F: Function> Env<'a, F> {
is_stack: false,
},
);
for &preg in &self.env.fixed_stack_slots {
for &preg in self.env.fixed_stack_slots {
self.pregs[preg.index()].is_stack = true;
}
for class in 0..self.preferred_victim_by_class.len() {
Expand Down
2 changes: 1 addition & 1 deletion src/ion/merge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use crate::{
};
use alloc::format;

impl<'a, F: Function> Env<'a, F> {
impl<'a, F: Function> Env<'a, '_, F> {
pub fn merge_bundles(&mut self, from: LiveBundleIndex, to: LiveBundleIndex) -> bool {
if from == to {
// Merge bundle into self -- trivial merge.
Expand Down
4 changes: 2 additions & 2 deletions src/ion/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ pub(crate) mod dump;
pub(crate) mod moves;
pub(crate) mod spill;

impl<'a, F: Function> Env<'a, F> {
pub(crate) fn new(func: &'a F, env: &'a MachineEnv, ctx: &'a mut Ctx) -> Self {
impl<'a, 'r, F: Function> Env<'a, 'r, F> {
pub(crate) fn new(func: &'a F, env: &'a MachineEnv<'r>, ctx: &'a mut Ctx) -> Self {
let ninstrs = func.num_insts();
let nblocks = func.num_blocks();

Expand Down
6 changes: 3 additions & 3 deletions src/ion/moves.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use alloc::vec::Vec;
use hashbrown::hash_map::Entry;
use smallvec::{smallvec, SmallVec};

impl<'a, F: Function> Env<'a, F> {
impl<'a, F: Function> Env<'a, '_, F> {
pub fn is_start_of_block(&self, pos: ProgPoint) -> bool {
let block = self.ctx.cfginfo.insn_block[pos.inst().index()];
pos == self.ctx.cfginfo.block_entry[block.index()]
Expand Down Expand Up @@ -171,7 +171,7 @@ impl<'a, F: Function> Env<'a, F> {
// and moves go at start of `to`.
#[inline(always)]
fn choose_move_location<'a, F: Function>(
env: &Env<'a, F>,
env: &Env<'a, '_, F>,
from: Block,
to: Block,
) -> (ProgPoint, InsertMovePrio) {
Expand Down Expand Up @@ -787,7 +787,7 @@ impl<'a, F: Function> Env<'a, F> {
let mut redundant_moves = RedundantMoveEliminator::default();

fn redundant_move_process_side_effects<'a, F: Function>(
this: &Env<'a, F>,
this: &Env<'a, '_, F>,
redundant_moves: &mut RedundantMoveEliminator,
from: ProgPoint,
to: ProgPoint,
Expand Down
2 changes: 1 addition & 1 deletion src/ion/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ pub enum AllocRegResult<'a> {
ConflictHighCost,
}

impl<'a, F: Function> Env<'a, F> {
impl<'a, F: Function> Env<'a, '_, F> {
pub fn process_bundles(&mut self) -> Result<(), RegAllocError> {
while let Some((bundle, reg_hint)) = self.ctx.allocation_queue.pop() {
self.ctx.output.stats.process_bundle_count += 1;
Expand Down
10 changes: 5 additions & 5 deletions src/ion/reg_traversal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ use crate::{MachineEnv, PReg, RegClass};
/// usage, these consist of caller-save and callee-save registers
/// respectively, to minimize clobber-saves; but they need not.)

pub struct RegTraversalIter<'a> {
env: &'a MachineEnv,
pub struct RegTraversalIter<'a, 'r> {
env: &'a MachineEnv<'r>,
class: usize,
hints: [Option<PReg>; 2],
hint_idx: usize,
Expand All @@ -27,9 +27,9 @@ pub struct RegTraversalIter<'a> {
fixed: Option<PReg>,
}

impl<'a> RegTraversalIter<'a> {
impl<'a, 'r> RegTraversalIter<'a, 'r> {
pub fn new(
env: &'a MachineEnv,
env: &'a MachineEnv<'r>,
class: RegClass,
hint_reg: PReg,
hint2_reg: PReg,
Expand Down Expand Up @@ -78,7 +78,7 @@ impl<'a> RegTraversalIter<'a> {
}
}

impl<'a> core::iter::Iterator for RegTraversalIter<'a> {
impl<'a> core::iter::Iterator for RegTraversalIter<'a, '_> {
type Item = PReg;

fn next(&mut self) -> Option<PReg> {
Expand Down
2 changes: 1 addition & 1 deletion src/ion/requirement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ impl Requirement {
}
}

impl<'a, F: Function> Env<'a, F> {
impl<'a, F: Function> Env<'a, '_, F> {
#[inline(always)]
pub fn requirement_from_operand(&self, op: Operand) -> Requirement {
match op.constraint() {
Expand Down
2 changes: 1 addition & 1 deletion src/ion/spill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use super::{
};
use crate::{Allocation, Function, SpillSlot};

impl<'a, F: Function> Env<'a, F> {
impl<'a, F: Function> Env<'a, '_, F> {
pub fn try_allocating_regs_for_spilled_bundles(&mut self) {
trace!("allocating regs for spilled bundles");
let mut scratch = core::mem::take(&mut self.ctx.scratch_conflicts);
Expand Down
21 changes: 13 additions & 8 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -334,17 +334,17 @@ impl Iterator for PRegSetIter {
}
}

impl From<&MachineEnv> for PRegSet {
impl From<&MachineEnv<'_>> for PRegSet {
fn from(env: &MachineEnv) -> Self {
let mut res = Self::default();

for class in env.preferred_regs_by_class.iter() {
for class in env.preferred_regs_by_class {
for preg in class {
res.add(*preg)
}
}

for class in env.non_preferred_regs_by_class.iter() {
for class in env.non_preferred_regs_by_class {
for preg in class {
res.add(*preg)
}
Expand Down Expand Up @@ -1434,15 +1434,20 @@ impl<'a> Iterator for OutputIter<'a> {
/// are available to allocate and what register may be used as a
/// scratch register for each class, and some other miscellaneous info
/// as well.
///
/// Note that `MachineEnv` is designed to be a global configuration struct that programs
/// will have very few of and generally want to keep around for their entire lifetime.
/// In order to make static initialization easier the registers lists are static slices instead
/// of `Vec`s. If your use case depends on dynamically creating the registers lists, consider
/// [`Vec::leak`].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not to give this suggestion in the doc-comment: since it's parameterized on a lifetime (rather than hardcoding 'static), it's possible to build a MachineEnv dynamically that is a borrowed view on owned Vecs, basically giving the same capabilities as before. This is fairly idiomatic in Rust, so I don't think we need a special note or recommendation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah good catch, that was from before I allowed any lifetimes. This notice doesn't make much sense anymore

#[derive(Clone, Debug)]
#[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
pub struct MachineEnv {
pub struct MachineEnv<'a> {
/// Preferred physical registers for each class. These are the
/// registers that will be allocated first, if free.
///
/// If an explicit scratch register is provided in `scratch_by_class` then
/// it must not appear in this list.
pub preferred_regs_by_class: [Vec<PReg>; 3],
pub preferred_regs_by_class: [&'a [PReg]; 3],

/// Non-preferred physical registers for each class. These are the
/// registers that will be allocated if a preferred register is
Expand All @@ -1451,7 +1456,7 @@ pub struct MachineEnv {
///
/// If an explicit scratch register is provided in `scratch_by_class` then
/// it must not appear in this list.
pub non_preferred_regs_by_class: [Vec<PReg>; 3],
pub non_preferred_regs_by_class: [&'a [PReg]; 3],

/// Optional dedicated scratch register per class. This is needed to perform
/// moves between registers when cyclic move patterns occur. The
Expand All @@ -1476,7 +1481,7 @@ pub struct MachineEnv {
///
/// `PReg`s in this list cannot be used as an allocatable or scratch
/// register.
pub fixed_stack_slots: Vec<PReg>,
pub fixed_stack_slots: &'a [PReg],
}

/// The output of the register allocator.
Expand Down
Loading
Loading