T0b1/wasmtime
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Partially rewrite the constant-phi-nodes pass to make it more idiomatic (#4111)

Browse Source 
* Add [must_use] to the timer functions

And remove unused vcode_post_ra function

* Make remove-constant-phis code more idiomatic
This commit is contained in:
Benjamin Bouvier
2022-05-09 19:22:34 +02:00
committed by GitHub
parent 4e6f3ea899
commit 8bd507db65
2 changed files with 60 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files

108
cranelift/codegen/src/remove_constant_phis.rs
View File

@@ -65,18 +65,20 @@ use std::vec::Vec;
// reducing the isel/regalloc cost downstream. Gains of up to 7% have been // reducing the isel/regalloc cost downstream. Gains of up to 7% have been
// seen for large functions. // seen for large functions.
// The `Value`s (Group B) that can flow to a formal parameter (Group A). /// The `Value`s (Group B) that can flow to a formal parameter (Group A).
#[derive(Clone, Copy, Debug, PartialEq)] #[derive(Clone, Copy, Debug, PartialEq)]
enum AbstractValue { enum AbstractValue {
// Two or more values flow to this formal. /// Two or more values flow to this formal.
Many, Many,
// Exactly one value, as stated, flows to this formal. The `Value`s that
// can appear here are exactly: `Value`s defined by `Inst`s, plus the /// Exactly one value, as stated, flows to this formal. The `Value`s that
// `Value`s defined by the formals of the entry block. Note that this is /// can appear here are exactly: `Value`s defined by `Inst`s, plus the
// exactly the set of `Value`s that are *not* tracked in the solver below /// `Value`s defined by the formals of the entry block. Note that this is
// (see `SolverState`). /// exactly the set of `Value`s that are *not* tracked in the solver below
/// (see `SolverState`).
One(Value /*Group B*/), One(Value /*Group B*/),
// No value flows to this formal.
/// No value flows to this formal.
None, None,
} }
@@ -99,28 +101,26 @@ impl AbstractValue {
} }
} }
} }
fn is_one(self) -> bool { fn is_one(self) -> bool {
if let AbstractValue::One(_) = self { matches!(self, AbstractValue::One(_))
true
} else {
false
}
} }
} }
// For some block, a useful bundle of info. The `Block` itself is not stored /// For some block, a useful bundle of info. The `Block` itself is not stored
// here since it will be the key in the associated `FxHashMap` -- see /// here since it will be the key in the associated `FxHashMap` -- see
// `summaries` below. For the `SmallVec` tuning params: most blocks have /// `summaries` below. For the `SmallVec` tuning params: most blocks have
// few parameters, hence `4`. And almost all blocks have either one or two /// few parameters, hence `4`. And almost all blocks have either one or two
// successors, hence `2`. /// successors, hence `2`.
#[derive(Debug)] #[derive(Debug)]
struct BlockSummary { struct BlockSummary {
// Formal parameters for this `Block` /// Formal parameters for this `Block`
formals: SmallVec<[Value; 4] /*Group A*/>, formals: SmallVec<[Value; 4] /*Group A*/>,
// For each `Inst` in this block that transfers to another block: the
// `Inst` itself, the destination `Block`, and the actual parameters /// For each `Inst` in this block that transfers to another block: the
// passed. We don't bother to include transfers that pass zero parameters /// `Inst` itself, the destination `Block`, and the actual parameters
// since that makes more work for the solver for no purpose. /// passed. We don't bother to include transfers that pass zero parameters
/// since that makes more work for the solver for no purpose.
dests: SmallVec<[(Inst, Block, SmallVec<[Value; 4] /*both Groups A and B*/>); 2]>, dests: SmallVec<[(Inst, Block, SmallVec<[Value; 4] /*both Groups A and B*/>); 2]>,
} }
impl BlockSummary { impl BlockSummary {
@@ -132,26 +132,30 @@ impl BlockSummary {
} }
} }
// Solver state. This holds a AbstractValue for each formal parameter, except /// Solver state. This holds a AbstractValue for each formal parameter, except
// for those from the entry block. /// for those from the entry block.
struct SolverState { struct SolverState {
absvals: FxHashMap<Value /*Group A*/, AbstractValue>, absvals: FxHashMap<Value /*Group A*/, AbstractValue>,
} }
impl SolverState { impl SolverState {
fn new() -> Self { fn new() -> Self {
Self { Self {
absvals: FxHashMap::default(), absvals: FxHashMap::default(),
} }
} }
fn get(&self, actual: Value) -> AbstractValue { fn get(&self, actual: Value) -> AbstractValue {
match self.absvals.get(&actual) { *self
Some(lp) => *lp, .absvals
None => panic!("SolverState::get: formal param {:?} is untracked?!", actual), .get(&actual)
} .unwrap_or_else(|| panic!("SolverState::get: formal param {:?} is untracked?!", actual))
} }
fn maybe_get(&self, actual: Value) -> Option<&AbstractValue> { fn maybe_get(&self, actual: Value) -> Option<&AbstractValue> {
self.absvals.get(&actual) self.absvals.get(&actual)
} }
fn set(&mut self, actual: Value, lp: AbstractValue) { fn set(&mut self, actual: Value, lp: AbstractValue) {
match self.absvals.insert(actual, lp) { match self.absvals.insert(actual, lp) {
Some(_old_lp) => {} Some(_old_lp) => {}
@@ -168,22 +172,22 @@ pub fn do_remove_constant_phis(func: &mut Function, domtree: &mut DominatorTree)
debug_assert!(domtree.is_valid()); debug_assert!(domtree.is_valid());
// Get the blocks, in reverse postorder // Get the blocks, in reverse postorder
let mut blocks_reverse_postorder = Vec::<Block>::new(); let blocks_reverse_postorder = domtree
for block in domtree.cfg_postorder() { .cfg_postorder()
blocks_reverse_postorder.push(*block); .into_iter()
} .rev()
blocks_reverse_postorder.reverse(); .collect::<Vec<_>>();
// Phase 1 of 3: for each block, make a summary containing all relevant // Phase 1 of 3: for each block, make a summary containing all relevant
// info. The solver will iterate over the summaries, rather than having // info. The solver will iterate over the summaries, rather than having
// to inspect each instruction in each block. // to inspect each instruction in each block.
let mut summaries = FxHashMap::<Block, BlockSummary>::default(); let mut summaries = FxHashMap::<Block, BlockSummary>::default();
for b in &blocks_reverse_postorder { for &&b in &blocks_reverse_postorder {
let formals = func.dfg.block_params(*b); let formals = func.dfg.block_params(b);
let mut summary = BlockSummary::new(SmallVec::from(formals)); let mut summary = BlockSummary::new(SmallVec::from(formals));
for inst in func.layout.block_insts(*b) { for inst in func.layout.block_insts(b) {
let idetails = &func.dfg[inst]; let idetails = &func.dfg[inst];
// Note that multi-dest transfers (i.e., branch tables) don't // Note that multi-dest transfers (i.e., branch tables) don't
// carry parameters in our IR, so we only have to care about // carry parameters in our IR, so we only have to care about
@@ -207,7 +211,7 @@ pub fn do_remove_constant_phis(func: &mut Function, domtree: &mut DominatorTree)
// in the summary, *unless* they have neither formals nor any // in the summary, *unless* they have neither formals nor any
// param-carrying branches/jumps. // param-carrying branches/jumps.
if formals.len() > 0 || summary.dests.len() > 0 { if formals.len() > 0 || summary.dests.len() > 0 {
summaries.insert(*b, summary); summaries.insert(b, summary);
} }
} }
@@ -223,12 +227,12 @@ pub fn do_remove_constant_phis(func: &mut Function, domtree: &mut DominatorTree)
// Set up initial solver state // Set up initial solver state
let mut state = SolverState::new(); let mut state = SolverState::new();
for b in &blocks_reverse_postorder { for &&b in &blocks_reverse_postorder {
// For each block, get the formals // For each block, get the formals
if *b == entry_block { if b == entry_block {
continue; continue;
} }
let formals: &[Value] = func.dfg.block_params(*b); let formals = func.dfg.block_params(b);
for formal in formals { for formal in formals {
let mb_old_absval = state.absvals.insert(*formal, AbstractValue::None); let mb_old_absval = state.absvals.insert(*formal, AbstractValue::None);
assert!(mb_old_absval.is_none()); assert!(mb_old_absval.is_none());
@@ -242,7 +246,7 @@ pub fn do_remove_constant_phis(func: &mut Function, domtree: &mut DominatorTree)
iter_no += 1; iter_no += 1;
let mut changed = false; let mut changed = false;
for src in &blocks_reverse_postorder { for &src in &blocks_reverse_postorder {
let mb_src_summary = summaries.get(src); let mb_src_summary = summaries.get(src);
// The src block might have no summary. This means it has no // The src block might have no summary. This means it has no
// branches/jumps that carry parameters *and* it doesn't take any // branches/jumps that carry parameters *and* it doesn't take any
@@ -261,7 +265,7 @@ pub fn do_remove_constant_phis(func: &mut Function, domtree: &mut DominatorTree)
.get(dst) .get(dst)
.expect("remove_constant_phis: dst block has no summary"); .expect("remove_constant_phis: dst block has no summary");
let dst_formals = &dst_summary.formals; let dst_formals = &dst_summary.formals;
assert!(src_actuals.len() == dst_formals.len()); assert_eq!(src_actuals.len(), dst_formals.len());
for (formal, actual) in dst_formals.iter().zip(src_actuals.iter()) { for (formal, actual) in dst_formals.iter().zip(src_actuals.iter()) {
// Find the abstract value for `actual`. If it is a block // Find the abstract value for `actual`. If it is a block
// formal parameter then the most recent abstract value is // formal parameter then the most recent abstract value is
@@ -288,6 +292,7 @@ pub fn do_remove_constant_phis(func: &mut Function, domtree: &mut DominatorTree)
break; break;
} }
} }
let mut n_consts = 0; let mut n_consts = 0;
for absval in state.absvals.values() { for absval in state.absvals.values() {
if absval.is_one() { if absval.is_one() {
@@ -327,11 +332,10 @@ pub fn do_remove_constant_phis(func: &mut Function, domtree: &mut DominatorTree)
} }
// We can delete the formals in any order. However, // We can delete the formals in any order. However,
// `remove_block_param` works by sliding backwards all arguments to // `remove_block_param` works by sliding backwards all arguments to
// the right of the it is asked to delete. Hence when removing more // the right of the value it is asked to delete. Hence when removing more
// than one formal, it is significantly more efficient to ask it to // than one formal, it is significantly more efficient to ask it to
// remove the rightmost formal first, and hence this `reverse`. // remove the rightmost formal first, and hence this `rev()`.
del_these.reverse(); for (redundant_formal, replacement_val) in del_these.into_iter().rev() {
for (redundant_formal, replacement_val) in del_these {
func.dfg.remove_block_param(redundant_formal); func.dfg.remove_block_param(redundant_formal);
func.dfg.change_to_alias(redundant_formal, replacement_val); func.dfg.change_to_alias(redundant_formal, replacement_val);
} }
@@ -356,7 +360,10 @@ pub fn do_remove_constant_phis(func: &mut Function, domtree: &mut DominatorTree)
// Check that the numbers of arguments make sense. // Check that the numbers of arguments make sense.
assert!(num_fixed_actuals <= num_old_actuals); assert!(num_fixed_actuals <= num_old_actuals);
assert!(num_fixed_actuals + dst_summary.formals.len() == num_old_actuals); assert_eq!(
num_fixed_actuals + dst_summary.formals.len(),
num_old_actuals
);
// Create a new value list. // Create a new value list.
let mut new_actuals = EntityList::<Value>::new(); let mut new_actuals = EntityList::<Value>::new();
@@ -368,12 +375,11 @@ pub fn do_remove_constant_phis(func: &mut Function, domtree: &mut DominatorTree)
// Copy the variable args (the actual block params) to the new // Copy the variable args (the actual block params) to the new
// list, filtering out redundant ones. // list, filtering out redundant ones.
for i in 0..dst_summary.formals.len() { for (i, formal_i) in dst_summary.formals.iter().enumerate() {
let actual_i = old_actuals let actual_i = old_actuals
.get(num_fixed_actuals + i, &func.dfg.value_lists) .get(num_fixed_actuals + i, &func.dfg.value_lists)
.unwrap(); .unwrap();
let formal_i = dst_summary.formals[i]; let is_redundant = state.get(*formal_i).is_one();
let is_redundant = state.get(formal_i).is_one();
if !is_redundant { if !is_redundant {
new_actuals.push(actual_i, &mut func.dfg.value_lists); new_actuals.push(actual_i, &mut func.dfg.value_lists);
} }

4
cranelift/codegen/src/timing.rs
View File

@@ -29,6 +29,7 @@ macro_rules! define_passes {
$( $(
#[doc=$desc] #[doc=$desc]
#[must_use]
pub fn $pass() -> TimingToken { pub fn $pass() -> TimingToken {
details::start_pass($enum::$pass) details::start_pass($enum::$pass)
} }
@@ -40,6 +41,8 @@ macro_rules! define_passes {
define_passes! { define_passes! {
Pass, NUM_PASSES, DESCRIPTIONS; Pass, NUM_PASSES, DESCRIPTIONS;
// All these are used in other crates but defined here so they appear in the unified
// `PassTimes` output.
process_file: "Processing test file", process_file: "Processing test file",
parse_text: "Parsing textual Cranelift IR", parse_text: "Parsing textual Cranelift IR",
wasm_translate_module: "Translate WASM module", wasm_translate_module: "Translate WASM module",
@@ -60,7 +63,6 @@ define_passes! {
remove_constant_phis: "Remove constant phi-nodes", remove_constant_phis: "Remove constant phi-nodes",
vcode_lower: "VCode lowering", vcode_lower: "VCode lowering",
vcode_post_ra: "VCode post-register allocation finalization",
vcode_emit: "VCode emission", vcode_emit: "VCode emission",
vcode_emit_finish: "VCode emission finalization", vcode_emit_finish: "VCode emission finalization",