diff --git a/cranelift/codegen/src/peepmatic.rs b/cranelift/codegen/src/peepmatic.rs index 24dd0981eb..ba9aeb38d5 100644 --- a/cranelift/codegen/src/peepmatic.rs +++ b/cranelift/codegen/src/peepmatic.rs @@ -422,7 +422,10 @@ fn peepmatic_ty_to_ir_ty(ty: Type, dfg: &DataFlowGraph, root: Inst) -> types::Ty } } -impl<'a, 'b> InstructionSet<'b> for &'a dyn TargetIsa { +// NB: the unsafe contract we must uphold here is that our implementation of +// `instruction_result_bit_width` must always return a valid, non-zero bit +// width. +unsafe impl<'a, 'b> InstructionSet<'b> for &'a dyn TargetIsa { type Context = FuncCursor<'b>; type Instruction = ValueOrInst; diff --git a/cranelift/codegen/src/preopt.serialized b/cranelift/codegen/src/preopt.serialized index eadd89e8cd..035de61c62 100644 Binary files a/cranelift/codegen/src/preopt.serialized and b/cranelift/codegen/src/preopt.serialized differ diff --git a/cranelift/peepmatic/crates/runtime/src/cc.rs b/cranelift/peepmatic/crates/runtime/src/cc.rs index 69f44db2ae..d6cb489eab 100644 --- a/cranelift/peepmatic/crates/runtime/src/cc.rs +++ b/cranelift/peepmatic/crates/runtime/src/cc.rs @@ -12,6 +12,8 @@ use std::fmt; #[repr(u32)] pub enum ConditionCode { /// Equal. + // NB: We convert `ConditionCode` into `NonZeroU32`s with unchecked + // conversions; memory safety relies on no variant being zero. Eq = 1, /// Not equal. diff --git a/cranelift/peepmatic/crates/runtime/src/instruction_set.rs b/cranelift/peepmatic/crates/runtime/src/instruction_set.rs index 1a501d4ce4..4d9a0752e5 100644 --- a/cranelift/peepmatic/crates/runtime/src/instruction_set.rs +++ b/cranelift/peepmatic/crates/runtime/src/instruction_set.rs @@ -21,7 +21,11 @@ use std::fmt::Debug; /// new `MachInst` and vcode backend easier, since all that needs to be done is /// "just" implementing this trait. (And probably add/modify some /// `peepmatic_runtime::operation::Operation`s as well). -pub trait InstructionSet<'a> { +/// +/// ## Safety +/// +/// See doc comment for `instruction_result_bit_width`. +pub unsafe trait InstructionSet<'a> { /// Mutable context passed into all trait methods. Can be whatever you want! /// /// In practice, this is a `FuncCursor` for `cranelift-codegen`'s trait @@ -124,7 +128,10 @@ pub trait InstructionSet<'a> { /// Get the bit width of the given instruction's result. /// - /// Must be one of 1, 8, 16, 32, 64, or 128. + /// ## Safety + /// + /// There is code that makes memory-safety assumptions that the result is + /// always one of 1, 8, 16, 32, 64, or 128. Implementors must uphold this. fn instruction_result_bit_width( &self, context: &mut Self::Context, diff --git a/cranelift/peepmatic/crates/runtime/src/integer_interner.rs b/cranelift/peepmatic/crates/runtime/src/integer_interner.rs index 93dd2e7183..c2f0830bf0 100644 --- a/cranelift/peepmatic/crates/runtime/src/integer_interner.rs +++ b/cranelift/peepmatic/crates/runtime/src/integer_interner.rs @@ -7,11 +7,11 @@ use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; -use std::convert::TryInto; +use std::num::NonZeroU32; /// An identifier for an interned integer. #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)] -pub struct IntegerId(#[doc(hidden)] pub u32); +pub struct IntegerId(#[doc(hidden)] pub NonZeroU32); /// An interner for integer values. #[derive(Debug, Default, Serialize, Deserialize)] @@ -40,7 +40,8 @@ impl IntegerInterner { return *id; } - let id = IntegerId(self.values.len().try_into().unwrap()); + assert!((self.values.len() as u64) < (std::u32::MAX as u64)); + let id = IntegerId(unsafe { NonZeroU32::new_unchecked(self.values.len() as u32 + 1) }); self.values.push(value); self.map.insert(value, id); @@ -59,13 +60,21 @@ impl IntegerInterner { /// Lookup a previously interned integer by id. #[inline] pub fn lookup(&self, id: IntegerId) -> u64 { - self.values[id.0 as usize] + let index = id.0.get() as usize - 1; + self.values[index] } } impl From for u32 { #[inline] fn from(id: IntegerId) -> u32 { + id.0.get() + } +} + +impl From for NonZeroU32 { + #[inline] + fn from(id: IntegerId) -> NonZeroU32 { id.0 } } diff --git a/cranelift/peepmatic/crates/runtime/src/linear.rs b/cranelift/peepmatic/crates/runtime/src/linear.rs index 006f18e0e5..fe98edb897 100644 --- a/cranelift/peepmatic/crates/runtime/src/linear.rs +++ b/cranelift/peepmatic/crates/runtime/src/linear.rs @@ -11,6 +11,7 @@ use crate::operator::{Operator, UnquoteOperator}; use crate::paths::{PathId, PathInterner}; use crate::r#type::{BitWidth, Type}; use serde::{Deserialize, Serialize}; +use std::num::NonZeroU32; /// A set of linear optimizations. #[derive(Debug)] @@ -32,6 +33,26 @@ pub struct Optimization { pub increments: Vec, } +/// Match any value. +/// +/// This can be used to create fallback, wildcard-style transitions between +/// states. +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] +pub struct Else; + +/// The result of evaluating a `MatchOp`. +/// +/// This is either a specific non-zero `u32`, or a fallback that matches +/// everything. +pub type MatchResult = Result; + +/// Convert a boolean to a `MatchResult`. +#[inline] +pub fn bool_to_match_result(b: bool) -> MatchResult { + let b = b as u32; + unsafe { Ok(NonZeroU32::new_unchecked(b + 1)) } +} + /// An increment is a matching operation, the expected result from that /// operation to continue to the next increment, and the actions to take to /// build up the LHS scope and RHS instructions given that we got the expected @@ -44,9 +65,9 @@ pub struct Increment { pub operation: MatchOp, /// The expected result of our matching operation, that enables us to - /// continue to the next increment. `None` is used for wildcard-style "else" - /// transitions. - pub expected: Option, + /// continue to the next increment, or `Else` for "don't care" + /// wildcard-style matching. + pub expected: MatchResult, /// Actions to perform, given that the operation resulted in the expected /// value. @@ -217,3 +238,23 @@ pub enum Action { operands: [RhsId; 3], }, } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn match_result_is_4_bytes_in_size() { + assert_eq!(std::mem::size_of::(), 4); + } + + #[test] + fn match_op_is_12_bytes_in_size() { + assert_eq!(std::mem::size_of::(), 12); + } + + #[test] + fn action_is_20_bytes_in_size() { + assert_eq!(std::mem::size_of::(), 20); + } +} diff --git a/cranelift/peepmatic/crates/runtime/src/operator.rs b/cranelift/peepmatic/crates/runtime/src/operator.rs index 35e17a89f7..c8f06b8514 100644 --- a/cranelift/peepmatic/crates/runtime/src/operator.rs +++ b/cranelift/peepmatic/crates/runtime/src/operator.rs @@ -20,6 +20,9 @@ use serde::{Deserialize, Serialize}; pub enum Operator { /// `adjust_sp_down` #[peepmatic(params(iNN), result(void))] + // NB: We convert `Operator`s into `NonZeroU32`s with unchecked casts; + // memory safety relies on `Operator` starting at `1` and no variant ever + // being zero. AdjustSpDown = 1, /// `adjust_sp_down_imm` diff --git a/cranelift/peepmatic/crates/runtime/src/optimizations.rs b/cranelift/peepmatic/crates/runtime/src/optimizations.rs index 29dfa7f388..4ed66a8a12 100644 --- a/cranelift/peepmatic/crates/runtime/src/optimizations.rs +++ b/cranelift/peepmatic/crates/runtime/src/optimizations.rs @@ -3,7 +3,7 @@ use crate::error::Result; use crate::instruction_set::InstructionSet; use crate::integer_interner::IntegerInterner; -use crate::linear::{Action, MatchOp}; +use crate::linear::{Action, MatchOp, MatchResult}; use crate::optimizer::PeepholeOptimizer; use crate::paths::PathInterner; use peepmatic_automata::Automaton; @@ -29,7 +29,7 @@ pub struct PeepholeOptimizations { /// The underlying automata for matching optimizations' left-hand sides, and /// building up the corresponding right-hand side. - pub automata: Automaton, MatchOp, Vec>, + pub automata: Automaton>, } impl PeepholeOptimizations { diff --git a/cranelift/peepmatic/crates/runtime/src/optimizer.rs b/cranelift/peepmatic/crates/runtime/src/optimizer.rs index 535bb56219..fc5916f247 100644 --- a/cranelift/peepmatic/crates/runtime/src/optimizer.rs +++ b/cranelift/peepmatic/crates/runtime/src/optimizer.rs @@ -1,7 +1,7 @@ //! An optimizer for a set of peephole optimizations. use crate::instruction_set::InstructionSet; -use crate::linear::{Action, MatchOp}; +use crate::linear::{bool_to_match_result, Action, Else, MatchOp, MatchResult}; use crate::operator::UnquoteOperator; use crate::optimizations::PeepholeOptimizations; use crate::part::{Constant, Part}; @@ -10,6 +10,7 @@ use peepmatic_automata::State; use std::convert::TryFrom; use std::fmt::{self, Debug}; use std::mem; +use std::num::NonZeroU32; /// A peephole optimizer instance that can apply a set of peephole /// optimizations to instructions. @@ -275,43 +276,72 @@ where context: &mut I::Context, root: I::Instruction, match_op: MatchOp, - ) -> Option { + ) -> MatchResult { use crate::linear::MatchOp::*; log::trace!("Evaluating match operation: {:?}", match_op); - let result = match match_op { + let result: MatchResult = (|| match match_op { Opcode { path } => { let path = self.peep_opt.paths.lookup(path); - let part = self.instr_set.get_part_at_path(context, root, path)?; - let inst = part.as_instruction()?; - self.instr_set.operator(context, inst).map(|op| op as u32) + let part = self + .instr_set + .get_part_at_path(context, root, path) + .ok_or(Else)?; + let inst = part.as_instruction().ok_or(Else)?; + let op = self.instr_set.operator(context, inst).ok_or(Else)?; + let op = op as u32; + debug_assert!( + op != 0, + "`Operator` doesn't have any variant represented + with zero" + ); + Ok(unsafe { NonZeroU32::new_unchecked(op as u32) }) } IsConst { path } => { let path = self.peep_opt.paths.lookup(path); - let part = self.instr_set.get_part_at_path(context, root, path)?; + let part = self + .instr_set + .get_part_at_path(context, root, path) + .ok_or(Else)?; let is_const = match part { Part::Instruction(i) => { self.instr_set.instruction_to_constant(context, i).is_some() } Part::ConditionCode(_) | Part::Constant(_) => true, }; - Some(is_const as u32) + bool_to_match_result(is_const) } IsPowerOfTwo { path } => { let path = self.peep_opt.paths.lookup(path); - let part = self.instr_set.get_part_at_path(context, root, path)?; + let part = self + .instr_set + .get_part_at_path(context, root, path) + .ok_or(Else)?; match part { - Part::Constant(c) => Some(c.as_int().unwrap().is_power_of_two() as u32), - Part::Instruction(i) => { - let c = self.instr_set.instruction_to_constant(context, i)?; - Some(c.as_int().unwrap().is_power_of_two() as u32) + Part::Constant(c) => { + let is_pow2 = c.as_int().unwrap().is_power_of_two(); + bool_to_match_result(is_pow2) } - Part::ConditionCode(_) => panic!("IsPowerOfTwo on a condition code"), + Part::Instruction(i) => { + let c = self + .instr_set + .instruction_to_constant(context, i) + .ok_or(Else)?; + let is_pow2 = c.as_int().unwrap().is_power_of_two(); + bool_to_match_result(is_pow2) + } + Part::ConditionCode(_) => unreachable!( + "IsPowerOfTwo on a condition + code" + ), } } BitWidth { path } => { let path = self.peep_opt.paths.lookup(path); - let part = self.instr_set.get_part_at_path(context, root, path)?; + let part = self + .instr_set + .get_part_at_path(context, root, path) + .ok_or(Else)?; let bit_width = match part { Part::Instruction(i) => self.instr_set.instruction_result_bit_width(context, i), Part::Constant(Constant::Int(_, w)) | Part::Constant(Constant::Bool(_, w)) => { @@ -321,14 +351,22 @@ where } Part::ConditionCode(_) => panic!("BitWidth on condition code"), }; - Some(bit_width as u32) + debug_assert!( + bit_width != 0, + "`InstructionSet` implementors must uphold the contract that \ + `instruction_result_bit_width` returns one of 1, 8, 16, 32, 64, or 128" + ); + Ok(unsafe { NonZeroU32::new_unchecked(bit_width as u32) }) } FitsInNativeWord { path } => { let native_word_size = self.instr_set.native_word_size_in_bits(context); debug_assert!(native_word_size.is_power_of_two()); let path = self.peep_opt.paths.lookup(path); - let part = self.instr_set.get_part_at_path(context, root, path)?; + let part = self + .instr_set + .get_part_at_path(context, root, path) + .ok_or(Else)?; let fits = match part { Part::Instruction(i) => { let size = self.instr_set.instruction_result_bit_width(context, i); @@ -341,13 +379,19 @@ where } Part::ConditionCode(_) => panic!("FitsInNativeWord on condition code"), }; - Some(fits as u32) + bool_to_match_result(fits) } Eq { path_a, path_b } => { let path_a = self.peep_opt.paths.lookup(path_a); - let part_a = self.instr_set.get_part_at_path(context, root, path_a)?; + let part_a = self + .instr_set + .get_part_at_path(context, root, path_a) + .ok_or(Else)?; let path_b = self.peep_opt.paths.lookup(path_b); - let part_b = self.instr_set.get_part_at_path(context, root, path_b)?; + let part_b = self + .instr_set + .get_part_at_path(context, root, path_b) + .ok_or(Else)?; let eq = match (part_a, part_b) { (Part::Instruction(inst), Part::Constant(c1)) | (Part::Constant(c1), Part::Instruction(inst)) => { @@ -358,43 +402,67 @@ where } (a, b) => a == b, }; - Some(eq as _) + bool_to_match_result(eq) } IntegerValue { path } => { let path = self.peep_opt.paths.lookup(path); - let part = self.instr_set.get_part_at_path(context, root, path)?; + let part = self + .instr_set + .get_part_at_path(context, root, path) + .ok_or(Else)?; match part { Part::Constant(c) => { - let x = c.as_int()?; - self.peep_opt.integers.already_interned(x).map(|id| id.0) + let x = c.as_int().ok_or(Else)?; + let id = self.peep_opt.integers.already_interned(x).ok_or(Else)?; + Ok(id.0) } Part::Instruction(i) => { - let c = self.instr_set.instruction_to_constant(context, i)?; - let x = c.as_int()?; - self.peep_opt.integers.already_interned(x).map(|id| id.0) + let c = self + .instr_set + .instruction_to_constant(context, i) + .ok_or(Else)?; + let x = c.as_int().ok_or(Else)?; + let id = self.peep_opt.integers.already_interned(x).ok_or(Else)?; + Ok(id.0) } - Part::ConditionCode(_) => panic!("IntegerValue on condition code"), + Part::ConditionCode(_) => unreachable!("IntegerValue on condition code"), } } BooleanValue { path } => { let path = self.peep_opt.paths.lookup(path); - let part = self.instr_set.get_part_at_path(context, root, path)?; + let part = self + .instr_set + .get_part_at_path(context, root, path) + .ok_or(Else)?; match part { - Part::Constant(c) => c.as_bool().map(|b| b as u32), - Part::Instruction(i) => { - let c = self.instr_set.instruction_to_constant(context, i)?; - c.as_bool().map(|b| b as u32) + Part::Constant(c) => { + let b = c.as_bool().ok_or(Else)?; + bool_to_match_result(b) } - Part::ConditionCode(_) => panic!("IntegerValue on condition code"), + Part::Instruction(i) => { + let c = self + .instr_set + .instruction_to_constant(context, i) + .ok_or(Else)?; + let b = c.as_bool().ok_or(Else)?; + bool_to_match_result(b) + } + Part::ConditionCode(_) => unreachable!("IntegerValue on condition code"), } } ConditionCode { path } => { let path = self.peep_opt.paths.lookup(path); - let part = self.instr_set.get_part_at_path(context, root, path)?; - part.as_condition_code().map(|cc| cc as u32) + let part = self + .instr_set + .get_part_at_path(context, root, path) + .ok_or(Else)?; + let cc = part.as_condition_code().ok_or(Else)?; + let cc = cc as u32; + debug_assert!(cc != 0); + Ok(unsafe { NonZeroU32::new_unchecked(cc) }) } - MatchOp::Nop => None, - }; + MatchOp::Nop => Err(Else), + })(); log::trace!("Evaluated match operation: {:?} = {:?}", match_op, result); result } @@ -437,12 +505,12 @@ where r#final = Some((query.current_state(), self.actions.len())); } - // Anything following a `None` transition doesn't care about the + // Anything following a `Else` transition doesn't care about the // result of this match operation, so if we partially follow the - // current non-`None` path, but don't ultimately find a matching + // current non-`Else` path, but don't ultimately find a matching // optimization, we want to be able to backtrack to this state and - // then try taking the `None` transition. - if query.has_transition_on(&None) { + // then try taking the `Else` transition. + if query.has_transition_on(&Err(Else)) { self.backtracking_states .push((query.current_state(), self.actions.len())); } @@ -462,8 +530,8 @@ where query.go_to_state(state); self.actions.truncate(actions_len); query - .next(&None) - .expect("backtracking states always have `None` transitions") + .next(&Err(Else)) + .expect("backtracking states always have `Else` transitions") } else { break; }; diff --git a/cranelift/peepmatic/crates/test/src/lib.rs b/cranelift/peepmatic/crates/test/src/lib.rs index c43ce89957..f74e9b7c53 100644 --- a/cranelift/peepmatic/crates/test/src/lib.rs +++ b/cranelift/peepmatic/crates/test/src/lib.rs @@ -307,7 +307,9 @@ pub struct TestIsa { pub native_word_size_in_bits: u8, } -impl<'a> InstructionSet<'a> for TestIsa { +// Unsafe because we must ensure that `instruction_result_bit_width` never +// returns zero. +unsafe impl<'a> InstructionSet<'a> for TestIsa { type Context = Program; type Instruction = Instruction; @@ -521,7 +523,9 @@ impl<'a> InstructionSet<'a> for TestIsa { fn instruction_result_bit_width(&self, program: &mut Program, inst: Instruction) -> u8 { log::debug!("instruction_result_bit_width({:?})", inst); let ty = program.data(inst).r#type; - ty.bit_width.fixed_width().unwrap() + let width = ty.bit_width.fixed_width().unwrap(); + assert!(width != 0); + width } fn native_word_size_in_bits(&self, _program: &mut Program) -> u8 { diff --git a/cranelift/peepmatic/src/automatize.rs b/cranelift/peepmatic/src/automatize.rs index 3310bef118..5b96339f9f 100644 --- a/cranelift/peepmatic/src/automatize.rs +++ b/cranelift/peepmatic/src/automatize.rs @@ -6,10 +6,10 @@ use peepmatic_runtime::linear; /// Construct an automaton from a set of linear optimizations. pub fn automatize( opts: &linear::Optimizations, -) -> Automaton, linear::MatchOp, Vec> { +) -> Automaton> { debug_assert!(crate::linear_passes::is_sorted_lexicographically(opts)); - let mut builder = Builder::, linear::MatchOp, Vec>::new(); + let mut builder = Builder::>::new(); for opt in &opts.optimizations { let mut insertion = builder.insert(); diff --git a/cranelift/peepmatic/src/dot_fmt.rs b/cranelift/peepmatic/src/dot_fmt.rs index a2c75de02c..959fdd96d9 100644 --- a/cranelift/peepmatic/src/dot_fmt.rs +++ b/cranelift/peepmatic/src/dot_fmt.rs @@ -12,36 +12,37 @@ use peepmatic_runtime::{ }; use std::convert::TryFrom; use std::io::{self, Write}; +use std::num::NonZeroU32; #[derive(Debug)] pub(crate) struct PeepholeDotFmt<'a>(pub(crate) &'a PathInterner, pub(crate) &'a IntegerInterner); -impl DotFmt, linear::MatchOp, Vec> for PeepholeDotFmt<'_> { +impl DotFmt> for PeepholeDotFmt<'_> { fn fmt_transition( &self, w: &mut impl Write, from: Option<&linear::MatchOp>, - input: &Option, + input: &linear::MatchResult, _to: Option<&linear::MatchOp>, ) -> io::Result<()> { let from = from.expect("we should have match op for every state"); - if let Some(x) = input { + if let Some(x) = input.ok().map(|x| x.get()) { match from { linear::MatchOp::Opcode { .. } => { let opcode = - Operator::try_from(*x).expect("we shouldn't generate non-opcode edges"); + Operator::try_from(x).expect("we shouldn't generate non-opcode edges"); write!(w, "{}", opcode) } linear::MatchOp::ConditionCode { .. } => { let cc = - ConditionCode::try_from(*x).expect("we shouldn't generate non-CC edges"); + ConditionCode::try_from(x).expect("we shouldn't generate non-CC edges"); write!(w, "{}", cc) } linear::MatchOp::IntegerValue { .. } => { - let x = self.1.lookup(IntegerId(*x)); + let x = self.1.lookup(IntegerId(NonZeroU32::new(x).unwrap())); write!(w, "{}", x) } - _ => write!(w, "{}", x), + _ => write!(w, "Ok({})", x), } } else { write!(w, "(else)") diff --git a/cranelift/peepmatic/src/linear_passes.rs b/cranelift/peepmatic/src/linear_passes.rs index cac1ca3d19..9e87ae0036 100644 --- a/cranelift/peepmatic/src/linear_passes.rs +++ b/cranelift/peepmatic/src/linear_passes.rs @@ -65,7 +65,12 @@ fn compare_optimizations( return c; } - let c = a.expected.cmp(&b.expected).reverse(); + let c = match (a.expected, b.expected) { + (Ok(a), Ok(b)) => a.cmp(&b).reverse(), + (Err(_), Ok(_)) => Ordering::Greater, + (Ok(_), Err(_)) => Ordering::Less, + (Err(linear::Else), Err(linear::Else)) => Ordering::Equal, + }; if c != Ordering::Equal { return c; } @@ -228,10 +233,10 @@ pub fn match_in_same_order(opts: &mut linear::Optimizations) { Some(_) => { new_increments.push(linear::Increment { operation: *last_op, - expected: None, + expected: Err(linear::Else), actions: vec![], }); - if last_expected.is_some() { + if last_expected.is_ok() { break; } } @@ -282,12 +287,99 @@ pub fn remove_unnecessary_nops(opts: &mut linear::Optimizations) { mod tests { use super::*; use crate::ast::*; - use linear::MatchOp::*; - use peepmatic_runtime::{operator::Operator, paths::*}; + use peepmatic_runtime::{ + linear::{bool_to_match_result, Else, MatchOp::*, MatchResult}, + operator::Operator, + paths::*, + }; + use std::num::NonZeroU32; + + #[test] + fn ok_non_zero_less_than_err_else() { + assert!(Ok(NonZeroU32::new(1).unwrap()) < Err(Else)); + } macro_rules! sorts_to { ($test_name:ident, $source:expr, $make_expected:expr) => { #[test] + #[allow(unused_variables)] + fn $test_name() { + let buf = wast::parser::ParseBuffer::new($source).expect("should lex OK"); + + let opts = match wast::parser::parse::(&buf) { + Ok(opts) => opts, + Err(mut e) => { + e.set_path(std::path::Path::new(stringify!($test_name))); + e.set_text($source); + eprintln!("{}", e); + panic!("should parse OK") + } + }; + + if let Err(mut e) = crate::verify(&opts) { + e.set_path(std::path::Path::new(stringify!($test_name))); + e.set_text($source); + eprintln!("{}", e); + panic!("should verify OK") + } + + let mut opts = crate::linearize(&opts); + + let before = opts + .optimizations + .iter() + .map(|o| { + o.increments + .iter() + .map(|i| format!("{:?} == {:?}", i.operation, i.expected)) + .collect::>() + }) + .collect::>(); + eprintln!("before = {:#?}", before); + + sort_least_to_most_general(&mut opts); + + let after = opts + .optimizations + .iter() + .map(|o| { + o.increments + .iter() + .map(|i| format!("{:?} == {:?}", i.operation, i.expected)) + .collect::>() + }) + .collect::>(); + eprintln!("after = {:#?}", before); + + let linear::Optimizations { + mut paths, + mut integers, + optimizations, + } = opts; + + let actual: Vec> = optimizations + .iter() + .map(|o| { + o.increments + .iter() + .map(|i| (i.operation, i.expected)) + .collect() + }) + .collect(); + + let mut p = |p: &[u8]| paths.intern(Path::new(&p)); + let mut i = |i: u64| Ok(integers.intern(i).into()); + let expected = $make_expected(&mut p, &mut i); + + assert_eq!(expected, actual); + } + }; + } + + macro_rules! match_in_same_order { + ($test_name:ident, $source:expr, $make_expected:expr) => { + #[test] + #[allow(unused_variables)] fn $test_name() { let buf = wast::parser::ParseBuffer::new($source).expect("should lex OK"); @@ -311,6 +403,32 @@ mod tests { let mut opts = crate::linearize(&opts); sort_least_to_most_general(&mut opts); + let before = opts + .optimizations + .iter() + .map(|o| { + o.increments + .iter() + .map(|i| format!("{:?} == {:?}", i.operation, i.expected)) + .collect::>() + }) + .collect::>(); + eprintln!("before = {:#?}", before); + + match_in_same_order(&mut opts); + + let after = opts + .optimizations + .iter() + .map(|o| { + o.increments + .iter() + .map(|i| format!("{:?} == {:?}", i.operation, i.expected)) + .collect::>() + }) + .collect::>(); + eprintln!("after = {:#?}", before); + let linear::Optimizations { mut paths, mut integers, @@ -328,7 +446,7 @@ mod tests { .collect(); let mut p = |p: &[u8]| paths.intern(Path::new(&p)); - let mut i = |i: u64| Some(integers.intern(i).into()); + let mut i = |i: u64| Ok(integers.intern(i).into()); let expected = $make_expected(&mut p, &mut i); assert_eq!(expected, actual); @@ -348,53 +466,83 @@ mod tests { (=> (iadd $x 42) 0) (=> (iadd $x (iadd $y $z)) 0) ", - |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> Option| vec![ + |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> MatchResult| vec![ vec![ - (Opcode { path: p(&[0]) }, Some(Operator::Iadd as _)), - (Nop, None), - (Opcode { path: p(&[0, 1]) }, Some(Operator::Iadd as _)), - (Nop, None), - (Nop, None), + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Iadd as _).unwrap()) + ), + (Nop, Err(Else)), + ( + Opcode { path: p(&[0, 1]) }, + Ok(NonZeroU32::new(Operator::Iadd as _).unwrap()) + ), + (Nop, Err(Else)), + (Nop, Err(Else)), ], vec![ - (Opcode { path: p(&[0]) }, Some(Operator::Iadd as _)), - (Nop, None), + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Iadd as _).unwrap()) + ), + (Nop, Err(Else)), (IntegerValue { path: p(&[0, 1]) }, i(42)) ], vec![ - (Opcode { path: p(&[0]) }, Some(Operator::Iadd as _)), - (Nop, None), - (IsConst { path: p(&[0, 1]) }, Some(1)), - (IsPowerOfTwo { path: p(&[0, 1]) }, Some(1)) + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Iadd as _).unwrap()) + ), + (Nop, Err(Else)), + (IsConst { path: p(&[0, 1]) }, bool_to_match_result(true)), + ( + IsPowerOfTwo { path: p(&[0, 1]) }, + bool_to_match_result(true) + ) ], vec![ - (Opcode { path: p(&[0]) }, Some(Operator::Iadd as _)), - (Nop, None), - (IsConst { path: p(&[0, 1]) }, Some(1)), - (BitWidth { path: p(&[0, 0]) }, Some(32)) + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Iadd as _).unwrap()) + ), + (Nop, Err(Else)), + (IsConst { path: p(&[0, 1]) }, bool_to_match_result(true)), + ( + BitWidth { path: p(&[0, 0]) }, + Ok(NonZeroU32::new(32).unwrap()) + ) ], vec![ - (Opcode { path: p(&[0]) }, Some(Operator::Iadd as _)), - (Nop, None), - (IsConst { path: p(&[0, 1]) }, Some(1)) + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Iadd as _).unwrap()) + ), + (Nop, Err(Else)), + (IsConst { path: p(&[0, 1]) }, bool_to_match_result(true)) ], vec![ - (Opcode { path: p(&[0]) }, Some(Operator::Iadd as _)), - (Nop, None), + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Iadd as _).unwrap()) + ), + (Nop, Err(Else)), ( Eq { path_a: p(&[0, 1]), path_b: p(&[0, 0]), }, - Some(1) + bool_to_match_result(true) ) ], vec![ - (Opcode { path: p(&[0]) }, Some(Operator::Iadd as _)), - (Nop, None), - (Nop, None), + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Iadd as _).unwrap()) + ), + (Nop, Err(Else)), + (Nop, Err(Else)), ], - vec![(Nop, None)] + vec![(Nop, Err(Else))] ] ); @@ -408,37 +556,164 @@ mod tests { (=> (imul 2 $x) (ishl $x 1)) (=> (imul $x 2) (ishl $x 1)) ", - |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> Option| vec![ + |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> MatchResult| vec![ vec![ - (Opcode { path: p(&[0]) }, Some(Operator::Imul as _)), + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Imul as _).unwrap()) + ), (IntegerValue { path: p(&[0, 0]) }, i(2)), - (Nop, None) + (Nop, Err(Else)) ], vec![ - (Opcode { path: p(&[0]) }, Some(Operator::Imul as _)), + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Imul as _).unwrap()) + ), (IntegerValue { path: p(&[0, 0]) }, i(1)), - (Nop, None) + (Nop, Err(Else)) ], vec![ - (Opcode { path: p(&[0]) }, Some(Operator::Imul as _)), - (Nop, None), + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Imul as _).unwrap()) + ), + (Nop, Err(Else)), (IntegerValue { path: p(&[0, 1]) }, i(2)) ], vec![ - (Opcode { path: p(&[0]) }, Some(Operator::Imul as _)), - (Nop, None), + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Imul as _).unwrap()) + ), + (Nop, Err(Else)), (IntegerValue { path: p(&[0, 1]) }, i(1)) ], vec![ - (Opcode { path: p(&[0]) }, Some(Operator::Iadd as _)), + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Iadd as _).unwrap()) + ), (IntegerValue { path: p(&[0, 0]) }, i(0)), - (Nop, None) + (Nop, Err(Else)) ], vec![ - (Opcode { path: p(&[0]) }, Some(Operator::Iadd as _)), - (Nop, None), + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Iadd as _).unwrap()) + ), + (Nop, Err(Else)), (IntegerValue { path: p(&[0, 1]) }, i(0)) ] ] ); + + sorts_to!( + sort_redundant_bor, + " + (=> (bor (bor $x $y) $x) + (bor $x $y)) + + (=> (bor (bor $x $y) $y) + (bor $x $y)) + ", + |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> MatchResult| vec![ + vec![ + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Bor as _).unwrap()) + ), + ( + Opcode { path: p(&[0, 0]) }, + Ok(NonZeroU32::new(Operator::Bor as _).unwrap()) + ), + (Nop, Err(Else)), + (Nop, Err(Else)), + ( + Eq { + path_a: p(&[0, 1]), + path_b: p(&[0, 0, 0]), + }, + bool_to_match_result(true) + ), + ], + vec![ + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Bor as _).unwrap()) + ), + ( + Opcode { path: p(&[0, 0]) }, + Ok(NonZeroU32::new(Operator::Bor as _).unwrap()) + ), + (Nop, Err(Else)), + (Nop, Err(Else)), + ( + Eq { + path_a: p(&[0, 1]), + path_b: p(&[0, 0, 1]), + }, + bool_to_match_result(true) + ), + ], + ] + ); + + match_in_same_order!( + match_in_same_order_redundant_bor, + " + (=> (bor (bor $x $y) $x) + (bor $x $y)) + + (=> (bor (bor $x $y) $y) + (bor $x $y)) + ", + |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> MatchResult| vec![ + vec![ + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Bor as _).unwrap()) + ), + ( + Opcode { path: p(&[0, 0]) }, + Ok(NonZeroU32::new(Operator::Bor as _).unwrap()) + ), + (Nop, Err(Else)), + (Nop, Err(Else)), + ( + Eq { + path_a: p(&[0, 1]), + path_b: p(&[0, 0, 0]), + }, + bool_to_match_result(true) + ), + ], + vec![ + ( + Opcode { path: p(&[0]) }, + Ok(NonZeroU32::new(Operator::Bor as _).unwrap()) + ), + ( + Opcode { path: p(&[0, 0]) }, + Ok(NonZeroU32::new(Operator::Bor as _).unwrap()) + ), + (Nop, Err(Else)), + (Nop, Err(Else)), + ( + Eq { + path_a: p(&[0, 1]), + path_b: p(&[0, 0, 0]), + }, + Err(Else), + ), + ( + Eq { + path_a: p(&[0, 1]), + path_b: p(&[0, 0, 1]), + }, + bool_to_match_result(true) + ), + ], + ] + ); } diff --git a/cranelift/peepmatic/src/linearize.rs b/cranelift/peepmatic/src/linearize.rs index 75205c93b9..5a47abcfcf 100644 --- a/cranelift/peepmatic/src/linearize.rs +++ b/cranelift/peepmatic/src/linearize.rs @@ -92,6 +92,7 @@ use peepmatic_runtime::{ paths::{Path, PathId, PathInterner}, }; use std::collections::BTreeMap; +use std::num::NonZeroU32; use wast::Id; /// Translate the given AST optimizations into linear optimizations. @@ -142,9 +143,12 @@ fn linearize_optimization( // increment that checks the instruction-being-matched's bit width. if let Pattern::Operation(Operation { r#type, .. }) = pattern { if let Some(w) = r#type.get().and_then(|ty| ty.bit_width.fixed_width()) { + debug_assert!(w != 0, "All fixed-width bit widths are non-zero"); + let expected = Ok(unsafe { NonZeroU32::new_unchecked(w as u32) }); + increments.push(linear::Increment { operation: linear::MatchOp::BitWidth { path }, - expected: Some(w as u32), + expected, actions: vec![], }); } @@ -432,7 +436,7 @@ impl Precondition<'_> { let path = lhs_id_to_path.unwrap_first_occurrence(&id); linear::Increment { operation: linear::MatchOp::IsPowerOfTwo { path }, - expected: Some(1), + expected: linear::bool_to_match_result(true), actions: vec![], } } @@ -451,12 +455,14 @@ impl Precondition<'_> { })) => *value, _ => unreachable!("checked in verification"), }; - debug_assert!(width <= 128); - debug_assert!((width as u8).is_power_of_two()); + + assert!(0 < width && width <= 128); + assert!((width as u8).is_power_of_two()); + let expected = Ok(unsafe { NonZeroU32::new_unchecked(width as u32) }); linear::Increment { operation: linear::MatchOp::BitWidth { path }, - expected: Some(width as u32), + expected, actions: vec![], } } @@ -469,7 +475,7 @@ impl Precondition<'_> { let path = lhs_id_to_path.unwrap_first_occurrence(&id); linear::Increment { operation: linear::MatchOp::FitsInNativeWord { path }, - expected: Some(1), + expected: linear::bool_to_match_result(true), actions: vec![], } } @@ -488,17 +494,21 @@ impl Pattern<'_> { integers: &mut IntegerInterner, lhs_id_to_path: &LhsIdToPath, path: PathId, - ) -> (linear::MatchOp, Option) { + ) -> (linear::MatchOp, linear::MatchResult) { match self { Pattern::ValueLiteral(ValueLiteral::Integer(Integer { value, .. })) => ( linear::MatchOp::IntegerValue { path }, - Some(integers.intern(*value as u64).into()), + Ok(integers.intern(*value as u64).into()), + ), + Pattern::ValueLiteral(ValueLiteral::Boolean(Boolean { value, .. })) => ( + linear::MatchOp::BooleanValue { path }, + linear::bool_to_match_result(*value), ), - Pattern::ValueLiteral(ValueLiteral::Boolean(Boolean { value, .. })) => { - (linear::MatchOp::BooleanValue { path }, Some(*value as u32)) - } Pattern::ValueLiteral(ValueLiteral::ConditionCode(ConditionCode { cc, .. })) => { - (linear::MatchOp::ConditionCode { path }, Some(*cc as u32)) + let cc = *cc as u32; + debug_assert!(cc != 0, "no `ConditionCode` variants are zero"); + let expected = Ok(unsafe { NonZeroU32::new_unchecked(cc) }); + (linear::MatchOp::ConditionCode { path }, expected) } Pattern::Constant(Constant { id, .. }) => { if let Some(path_b) = lhs_id_to_path.get_first_occurrence(id) { @@ -508,10 +518,13 @@ impl Pattern<'_> { path_a: path, path_b, }, - Some(1), + linear::bool_to_match_result(true), ) } else { - (linear::MatchOp::IsConst { path }, Some(1)) + ( + linear::MatchOp::IsConst { path }, + linear::bool_to_match_result(true), + ) } } Pattern::Variable(Variable { id, .. }) => { @@ -522,13 +535,18 @@ impl Pattern<'_> { path_a: path, path_b, }, - Some(1), + linear::bool_to_match_result(true), ) } else { - (linear::MatchOp::Nop, None) + (linear::MatchOp::Nop, Err(linear::Else)) } } - Pattern::Operation(op) => (linear::MatchOp::Opcode { path }, Some(op.operator as u32)), + Pattern::Operation(op) => { + let op = op.operator as u32; + debug_assert!(op != 0, "no `Operator` variants are zero"); + let expected = Ok(unsafe { NonZeroU32::new_unchecked(op) }); + (linear::MatchOp::Opcode { path }, expected) + } } } } @@ -538,7 +556,7 @@ mod tests { use super::*; use peepmatic_runtime::{ integer_interner::IntegerId, - linear::{Action::*, MatchOp::*}, + linear::{bool_to_match_result, Action::*, Else, MatchOp::*}, operator::Operator, r#type::{BitWidth, Kind, Type}, }; @@ -603,7 +621,7 @@ mod tests { increments: vec![ linear::Increment { operation: Opcode { path: p(&[0]) }, - expected: Some(Operator::Imul as _), + expected: Ok(NonZeroU32::new(Operator::Imul as _).unwrap()), actions: vec![ GetLhs { path: p(&[0, 0]) }, GetLhs { path: p(&[0, 1]) }, @@ -619,17 +637,17 @@ mod tests { }, linear::Increment { operation: Nop, - expected: None, + expected: Err(Else), actions: vec![], }, linear::Increment { operation: IsConst { path: p(&[0, 1]) }, - expected: Some(1), + expected: bool_to_match_result(true), actions: vec![], }, linear::Increment { operation: IsPowerOfTwo { path: p(&[0, 1]) }, - expected: Some(1), + expected: bool_to_match_result(true), actions: vec![], }, ], @@ -644,7 +662,7 @@ mod tests { linear::Optimization { increments: vec![linear::Increment { operation: Nop, - expected: None, + expected: Err(Else), actions: vec![GetLhs { path: p(&[0]) }], }], } @@ -658,7 +676,7 @@ mod tests { linear::Optimization { increments: vec![linear::Increment { operation: IsConst { path: p(&[0]) }, - expected: Some(1), + expected: bool_to_match_result(true), actions: vec![GetLhs { path: p(&[0]) }], }], } @@ -672,7 +690,7 @@ mod tests { linear::Optimization { increments: vec![linear::Increment { operation: BooleanValue { path: p(&[0]) }, - expected: Some(1), + expected: bool_to_match_result(true), actions: vec![MakeBooleanConst { value: true, bit_width: BitWidth::Polymorphic, @@ -689,7 +707,7 @@ mod tests { linear::Optimization { increments: vec![linear::Increment { operation: IntegerValue { path: p(&[0]) }, - expected: Some(i(5).into()), + expected: Ok(i(5).into()), actions: vec![MakeIntegerConst { value: i(5), bit_width: BitWidth::Polymorphic, @@ -707,7 +725,7 @@ mod tests { increments: vec![ linear::Increment { operation: Opcode { path: p(&[0]) }, - expected: Some(Operator::Iconst as _), + expected: Ok(NonZeroU32::new(Operator::Iconst as _).unwrap()), actions: vec![ GetLhs { path: p(&[0, 0]) }, MakeUnaryInst { @@ -722,7 +740,7 @@ mod tests { }, linear::Increment { operation: IsConst { path: p(&[0, 0]) }, - expected: Some(1), + expected: bool_to_match_result(true), actions: vec![], }, ], @@ -738,7 +756,7 @@ mod tests { increments: vec![ linear::Increment { operation: Opcode { path: p(&[0]) }, - expected: Some(Operator::Bor as _), + expected: Ok(NonZeroU32::new(Operator::Bor as _).unwrap()), actions: vec![ GetLhs { path: p(&[0, 0]) }, GetLhs { @@ -756,12 +774,12 @@ mod tests { }, linear::Increment { operation: Nop, - expected: None, + expected: Err(Else), actions: vec![], }, linear::Increment { operation: Opcode { path: p(&[0, 1]) }, - expected: Some(Operator::Bor as _), + expected: Ok(NonZeroU32::new(Operator::Bor as _).unwrap()), actions: vec![], }, linear::Increment { @@ -769,12 +787,12 @@ mod tests { path_a: p(&[0, 1, 0]), path_b: p(&[0, 0]), }, - expected: Some(1), + expected: bool_to_match_result(true), actions: vec![], }, linear::Increment { operation: Nop, - expected: None, + expected: Err(Else), actions: vec![], }, ], @@ -790,7 +808,7 @@ mod tests { linear::Optimization { increments: vec![linear::Increment { operation: IntegerValue { path: p(&[0]) }, - expected: Some(i(std::u64::MAX).into()), + expected: Ok(i(std::u64::MAX).into()), actions: vec![MakeIntegerConst { value: i(0), bit_width: BitWidth::Polymorphic, @@ -808,7 +826,7 @@ mod tests { increments: vec![ linear::Increment { operation: Opcode { path: p(&[0]) }, - expected: Some(Operator::Ireduce as _), + expected: Ok(NonZeroU32::new(Operator::Ireduce as _).unwrap()), actions: vec![MakeIntegerConst { value: i(0), bit_width: BitWidth::ThirtyTwo, @@ -816,12 +834,12 @@ mod tests { }, linear::Increment { operation: linear::MatchOp::BitWidth { path: p(&[0]) }, - expected: Some(32), + expected: Ok(NonZeroU32::new(32).unwrap()), actions: vec![], }, linear::Increment { operation: Nop, - expected: None, + expected: Err(Else), actions: vec![], }, ],