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

Make DataFlowGraph::insts public, but restricted (#5450)

Browse Source 
We have some operations defined on DataFlowGraph purely to work around borrow-checker issues with InstructionData and other data on DataFlowGraph. Part of the problem is that indexing the DFG directly hides the fact that we're only indexing the insts field of the DFG.

This PR makes the insts field of the DFG public, but wraps it in a newtype that only allows indexing. This means that the borrow checker is better able to tell when operations on memory held by the DFG won't conflict, which comes up frequently when mutating ValueLists held by InstructionData.
This commit is contained in:
Trevor Elliott
2022-12-16 10:46:09 -08:00
committed by GitHub
parent 6323b0f9f4
commit 25bf8e0e67
31 changed files with 182 additions and 178 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
cranelift/codegen/src/ir/builder.rs
View File

@@ -201,7 +201,7 @@ impl<'f> InstBuilderBase<'f> for ReplaceBuilder<'f> {
fn build(self, data: InstructionData, ctrl_typevar: Type) -> (Inst, &'f mut DataFlowGraph) {
// Splat the new instruction on top of the old one.
self.dfg[self.inst] = data;
self.dfg.insts[self.inst] = data;
if !self.dfg.has_results(self.inst) {
// The old result values were either detached or non-existent.

80
cranelift/codegen/src/ir/dfg.rs
View File

@@ -23,6 +23,27 @@ use alloc::collections::BTreeMap;
#[cfg(feature = "enable-serde")]
use serde::{Deserialize, Serialize};
/// Storage for instructions within the DFG.
#[derive(Clone, PartialEq, Hash)]
#[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
pub struct Insts(PrimaryMap<Inst, InstructionData>);
/// Allow immutable access to instructions via indexing.
impl Index<Inst> for Insts {
type Output = InstructionData;
fn index(&self, inst: Inst) -> &InstructionData {
self.0.index(inst)
}
}
/// Allow mutable access to instructions via indexing.
impl IndexMut<Inst> for Insts {
fn index_mut(&mut self, inst: Inst) -> &mut InstructionData {
self.0.index_mut(inst)
}
}
/// A data flow graph defines all instructions and basic blocks in a function as well as
/// the data flow dependencies between them. The DFG also tracks values which can be either
/// instruction results or block parameters.
@@ -36,7 +57,7 @@ pub struct DataFlowGraph {
/// Data about all of the instructions in the function, including opcodes and operands.
/// The instructions in this map are not in program order. That is tracked by `Layout`, along
/// with the block containing each instruction.
insts: PrimaryMap<Inst, InstructionData>,
pub insts: Insts,
/// List of result values for each instruction.
///
@@ -89,7 +110,7 @@ impl DataFlowGraph {
/// Create a new empty `DataFlowGraph`.
pub fn new() -> Self {
Self {
insts: PrimaryMap::new(),
insts: Insts(PrimaryMap::new()),
results: SecondaryMap::new(),
blocks: PrimaryMap::new(),
dynamic_types: DynamicTypes::new(),
@@ -106,7 +127,7 @@ impl DataFlowGraph {
/// Clear everything.
pub fn clear(&mut self) {
self.insts.clear();
self.insts.0.clear();
self.results.clear();
self.blocks.clear();
self.dynamic_types.clear();
@@ -125,12 +146,12 @@ impl DataFlowGraph {
///
/// This is intended for use with `SecondaryMap::with_capacity`.
pub fn num_insts(&self) -> usize {
self.insts.len()
self.insts.0.len()
}
/// Returns `true` if the given instruction reference is valid.
pub fn inst_is_valid(&self, inst: Inst) -> bool {
self.insts.is_valid(inst)
self.insts.0.is_valid(inst)
}
/// Get the total number of basic blocks created in this function, whether they are
@@ -619,7 +640,7 @@ impl DataFlowGraph {
pub fn make_inst(&mut self, data: InstructionData) -> Inst {
let n = self.num_insts() + 1;
self.results.resize(n);
self.insts.push(data)
self.insts.0.push(data)
}
/// Declares a dynamic vector type
@@ -656,7 +677,7 @@ impl DataFlowGraph {
/// Get the fixed value arguments on `inst` as a slice.
pub fn inst_fixed_args(&self, inst: Inst) -> &[Value] {
let num_fixed_args = self[inst]
let num_fixed_args = self.insts[inst]
.opcode()
.constraints()
.num_fixed_value_arguments();
@@ -665,7 +686,7 @@ impl DataFlowGraph {
/// Get the fixed value arguments on `inst` as a mutable slice.
pub fn inst_fixed_args_mut(&mut self, inst: Inst) -> &mut [Value] {
let num_fixed_args = self[inst]
let num_fixed_args = self.insts[inst]
.opcode()
.constraints()
.num_fixed_value_arguments();
@@ -674,7 +695,7 @@ impl DataFlowGraph {
/// Get the variable value arguments on `inst` as a slice.
pub fn inst_variable_args(&self, inst: Inst) -> &[Value] {
let num_fixed_args = self[inst]
let num_fixed_args = self.insts[inst]
.opcode()
.constraints()
.num_fixed_value_arguments();
@@ -683,7 +704,7 @@ impl DataFlowGraph {
/// Get the variable value arguments on `inst` as a mutable slice.
pub fn inst_variable_args_mut(&mut self, inst: Inst) -> &mut [Value] {
let num_fixed_args = self[inst]
let num_fixed_args = self.insts[inst]
.opcode()
.constraints()
.num_fixed_value_arguments();
@@ -849,18 +870,17 @@ impl DataFlowGraph {
///
/// Panics if the instruction doesn't support arguments.
pub fn append_inst_arg(&mut self, inst: Inst, new_arg: Value) {
let mut branch_values = self.insts[inst]
.take_value_list()
.expect("the instruction doesn't have value arguments");
branch_values.push(new_arg, &mut self.value_lists);
self.insts[inst].put_value_list(branch_values)
self.insts[inst]
.value_list_mut()
.expect("the instruction doesn't have value arguments")
.push(new_arg, &mut self.value_lists);
}
/// Clone an instruction, attaching new result `Value`s and
/// returning them.
pub fn clone_inst(&mut self, inst: Inst) -> Inst {
// First, add a clone of the InstructionData.
let inst_data = self[inst].clone();
let inst_data = self.insts[inst].clone();
let new_inst = self.make_inst(inst_data);
// Get the controlling type variable.
let ctrl_typevar = self.ctrl_typevar(inst);
@@ -947,7 +967,7 @@ impl DataFlowGraph {
/// Get the controlling type variable, or `INVALID` if `inst` isn't polymorphic.
pub fn ctrl_typevar(&self, inst: Inst) -> Type {
let constraints = self[inst].opcode().constraints();
let constraints = self.insts[inst].opcode().constraints();
if !constraints.is_polymorphic() {
types::INVALID
@@ -955,12 +975,12 @@ impl DataFlowGraph {
// Not all instruction formats have a designated operand, but in that case
// `requires_typevar_operand()` should never be true.
self.value_type(
self[inst]
self.insts[inst]
.typevar_operand(&self.value_lists)
.unwrap_or_else(|| {
panic!(
"Instruction format for {:?} doesn't have a designated operand",
self[inst]
self.insts[inst]
)
}),
)
@@ -970,22 +990,6 @@ impl DataFlowGraph {
}
}
/// Allow immutable access to instructions via indexing.
impl Index<Inst> for DataFlowGraph {
type Output = InstructionData;
fn index(&self, inst: Inst) -> &InstructionData {
&self.insts[inst]
}
}
/// Allow mutable access to instructions via indexing.
impl IndexMut<Inst> for DataFlowGraph {
fn index_mut(&mut self, inst: Inst) -> &mut InstructionData {
&mut self.insts[inst]
}
}
/// basic blocks.
impl DataFlowGraph {
/// Create a new basic block.
@@ -1187,9 +1191,9 @@ impl<'a> fmt::Display for DisplayInst<'a> {
let typevar = dfg.ctrl_typevar(inst);
if typevar.is_invalid() {
write!(f, "{}", dfg[inst].opcode())?;
write!(f, "{}", dfg.insts[inst].opcode())?;
} else {
write!(f, "{}.{}", dfg[inst].opcode(), typevar)?;
write!(f, "{}.{}", dfg.insts[inst].opcode(), typevar)?;
}
write_operands(f, dfg, inst)
}
@@ -1376,7 +1380,7 @@ mod tests {
// Immutable reference resolution.
{
let immdfg = &dfg;
let ins = &immdfg[inst];
let ins = &immdfg.insts[inst];
assert_eq!(ins.opcode(), Opcode::Iconst);
}

10
cranelift/codegen/src/ir/function.rs
View File

@@ -282,7 +282,7 @@ impl FunctionStencil {
///
/// Note that this method ignores multi-destination branches like `br_table`.
pub fn change_branch_destination(&mut self, inst: Inst, new_dest: Block) {
match self.dfg[inst].branch_destination_mut() {
match self.dfg.insts[inst].branch_destination_mut() {
None => (),
Some(inst_dest) => *inst_dest = new_dest,
}
@@ -309,7 +309,7 @@ impl FunctionStencil {
});
if default_dest == Some(old_dest) {
match &mut self.dfg[inst] {
match &mut self.dfg.insts[inst] {
InstructionData::BranchTable { destination, .. } => {
*destination = new_dest;
}
@@ -333,13 +333,13 @@ impl FunctionStencil {
let inst_iter = self.layout.block_insts(block);
// Ignore all instructions prior to the first branch.
let mut inst_iter = inst_iter.skip_while(|&inst| !dfg[inst].opcode().is_branch());
let mut inst_iter = inst_iter.skip_while(|&inst| !dfg.insts[inst].opcode().is_branch());
// A conditional branch is permitted in a basic block only when followed
// by a terminal jump instruction.
if let Some(_branch) = inst_iter.next() {
if let Some(next) = inst_iter.next() {
match dfg[next].opcode() {
match dfg.insts[next].opcode() {
Opcode::Jump => (),
_ => return Err((next, "post-branch instruction not jump")),
}
@@ -377,7 +377,7 @@ impl FunctionStencil {
.zip(self.dfg.inst_results(src))
.all(|(a, b)| self.dfg.value_type(*a) == self.dfg.value_type(*b)));
self.dfg[dst] = self.dfg[src];
self.dfg.insts[dst] = self.dfg.insts[src];
self.layout.remove_inst(src);
}

2
cranelift/codegen/src/ir/layout.rs
View File

@@ -600,7 +600,7 @@ impl Layout {
// If two, the former is conditional and the latter is unconditional.
let last = self.last_inst(block)?;
if let Some(prev) = self.prev_inst(last) {
if dfg[prev].opcode().is_branch() {
if dfg.insts[prev].opcode().is_branch() {
return Some(prev);
}
}