-
Notifications
You must be signed in to change notification settings - Fork 225
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
feat(ssa): Treat globals as constant in a function's DFG #7040
base: master
Are you sure you want to change the base?
Changes from 35 commits
c7d8009
4b19a7b
c8e08d3
8a24746
e3358e3
c3948fe
328004e
40c50fc
7744179
68ddd14
6823019
73ceea5
cbe377e
2d750fb
faeb30f
2322cb6
08a2aba
fa6d9cf
200964f
f98c713
ad19fc3
bb5a9ea
74a7358
df5eef0
584cc92
8ff2934
aae17de
8a052b0
e14ad65
2db6171
12c4d3b
dcef98d
e166f24
9b8122b
e05405c
433da77
a7a8e8a
381ba57
2bbed37
e54caf6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
use std::borrow::Cow; | ||
use std::{borrow::Cow, sync::Arc}; | ||
|
||
use crate::ssa::{function_builder::data_bus::DataBus, ir::instruction::SimplifyResult}; | ||
|
||
|
@@ -102,6 +102,31 @@ pub(crate) struct DataFlowGraph { | |
|
||
#[serde(skip)] | ||
pub(crate) data_bus: DataBus, | ||
|
||
pub(crate) globals: Arc<GlobalsGraph>, | ||
} | ||
|
||
/// The GlobalsGraph contains the actual global data. | ||
/// Global data is expected to only be numeric constants or array constants (which are represented by Instruction::MakeArray). | ||
/// The global's data will shared across functions and should be accessible inside of a function's DataFlowGraph. | ||
#[derive(Debug, Clone, Serialize, Deserialize, Default)] | ||
pub(crate) struct GlobalsGraph { | ||
/// Storage for all of the global values | ||
values: DenseMap<Value>, | ||
/// All of the instructions in the global value space. | ||
/// These are expected to all be Instruction::MakeArray | ||
instructions: DenseMap<Instruction>, | ||
} | ||
|
||
impl GlobalsGraph { | ||
pub(crate) fn from_dfg(dfg: DataFlowGraph) -> Self { | ||
Self { values: dfg.values, instructions: dfg.instructions } | ||
} | ||
|
||
/// Iterate over every Value in this DFG in no particular order, including unused Values | ||
pub(crate) fn values_iter(&self) -> impl ExactSizeIterator<Item = (ValueId, &Value)> { | ||
self.values.iter() | ||
} | ||
} | ||
|
||
impl DataFlowGraph { | ||
|
@@ -511,7 +536,7 @@ impl DataFlowGraph { | |
&self, | ||
value: ValueId, | ||
) -> Option<(FieldElement, NumericType)> { | ||
match &self.values[self.resolve(value)] { | ||
match &self[self.resolve(value)] { | ||
Value::NumericConstant { constant, typ } => Some((*constant, *typ)), | ||
_ => None, | ||
} | ||
|
@@ -520,13 +545,15 @@ impl DataFlowGraph { | |
/// Returns the Value::Array associated with this ValueId if it refers to an array constant. | ||
/// Otherwise, this returns None. | ||
pub(crate) fn get_array_constant(&self, value: ValueId) -> Option<(im::Vector<ValueId>, Type)> { | ||
match &self.values[self.resolve(value)] { | ||
Value::Instruction { instruction, .. } => match &self.instructions[*instruction] { | ||
let value = self.resolve(value); | ||
if let Some(instruction) = self.get_instruction(value) { | ||
match instruction { | ||
Instruction::MakeArray { elements, typ } => Some((elements.clone(), typ.clone())), | ||
_ => None, | ||
}, | ||
} | ||
} else { | ||
// Arrays are shared, so cloning them is cheap | ||
_ => None, | ||
None | ||
} | ||
} | ||
|
||
|
@@ -619,17 +646,21 @@ impl DataFlowGraph { | |
|
||
/// True if the given ValueId refers to a (recursively) constant value | ||
pub(crate) fn is_constant(&self, argument: ValueId) -> bool { | ||
match &self[self.resolve(argument)] { | ||
let argument = self.resolve(argument); | ||
if self.is_global(argument) { | ||
return true; | ||
} | ||
match &self[argument] { | ||
Value::Param { .. } => false, | ||
Value::Instruction { instruction, .. } => match &self[*instruction] { | ||
Instruction::MakeArray { elements, .. } => { | ||
elements.iter().all(|element| self.is_constant(*element)) | ||
} | ||
_ => false, | ||
}, | ||
// TODO: Make this true and handle instruction simplifications with globals. | ||
// Currently all globals are inlined as a temporary measure so this is fine to have as false. | ||
Value::Global(_) => false, | ||
Value::Global(_) => { | ||
unreachable!("The global value case should already be handled"); | ||
} | ||
_ => true, | ||
} | ||
} | ||
|
@@ -642,6 +673,24 @@ impl DataFlowGraph { | |
false | ||
} | ||
} | ||
|
||
pub(crate) fn is_global(&self, value: ValueId) -> bool { | ||
matches!(self.values[value], Value::Global(_)) | ||
} | ||
|
||
pub(crate) fn get_instruction(&self, value: ValueId) -> Option<&Instruction> { | ||
match &self[value] { | ||
asterite marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Value::Instruction { instruction, .. } => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this method needs some clarification in both a doc comment and making the name more verbose to highlight that the returned instruction may not be in this DFG (!) since using the valueids directly from it could result in different underlying values from this DFG. As-is I can see someone using this as a general purpose way to get an instruction in an optimization pass and run into that bug without knowing - and it'd be hard to track down. Edit: I realize this is mostly mitigated by the Value indexing change below. So we'd only encounter this if we try to retrieve a value by accessing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add a safety comment either way. How does the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made the rename to |
||
let instruction = if self.is_global(value) { | ||
&self.globals[*instruction] | ||
} else { | ||
&self[*instruction] | ||
}; | ||
Some(instruction) | ||
} | ||
_ => None, | ||
} | ||
} | ||
} | ||
|
||
impl std::ops::Index<InstructionId> for DataFlowGraph { | ||
|
@@ -660,7 +709,11 @@ impl std::ops::IndexMut<InstructionId> for DataFlowGraph { | |
impl std::ops::Index<ValueId> for DataFlowGraph { | ||
type Output = Value; | ||
fn index(&self, id: ValueId) -> &Self::Output { | ||
&self.values[id] | ||
let value = &self.values[id]; | ||
if matches!(value, Value::Global(_)) { | ||
return &self.globals[id]; | ||
} | ||
value | ||
} | ||
Comment on lines
+719
to
+723
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, if we're checking here then my last comment shouldn't be as much of an issue - but I'll leave it anyway since I think it's an important point to highlight. For arrays of integers, all those integers should be Value::Globals as well, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes for a global array of integers. Right now the globals we declare are only dictated by user code, however, in the future we may want to hoist constants into the global space. |
||
} | ||
|
||
|
@@ -678,6 +731,20 @@ impl std::ops::IndexMut<BasicBlockId> for DataFlowGraph { | |
} | ||
} | ||
|
||
impl std::ops::Index<ValueId> for GlobalsGraph { | ||
type Output = Value; | ||
fn index(&self, id: ValueId) -> &Self::Output { | ||
&self.values[id] | ||
} | ||
} | ||
|
||
impl std::ops::Index<InstructionId> for GlobalsGraph { | ||
type Output = Instruction; | ||
fn index(&self, id: InstructionId) -> &Self::Output { | ||
&self.instructions[id] | ||
} | ||
} | ||
|
||
// The result of calling DataFlowGraph::insert_instruction can | ||
// be a list of results or a single ValueId if the instruction was simplified | ||
// to an existing value. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1093,28 +1093,27 @@ fn try_optimize_array_get_from_previous_set( | |
// Arbitrary number of maximum tries just to prevent this optimization from taking too long. | ||
let max_tries = 5; | ||
for _ in 0..max_tries { | ||
match &dfg[array_id] { | ||
Value::Instruction { instruction, .. } => { | ||
match &dfg[*instruction] { | ||
Instruction::ArraySet { array, index, value, .. } => { | ||
if let Some(constant) = dfg.get_numeric_constant(*index) { | ||
if constant == target_index { | ||
return SimplifyResult::SimplifiedTo(*value); | ||
} | ||
|
||
array_id = *array; // recur | ||
} else { | ||
return SimplifyResult::None; | ||
if let Some(instruction) = dfg.get_instruction(array_id) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we expect array_set to be used on global arrays at all? I'd expect/hope this wouldn't be needed here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is needed for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added an insertion inside of the dfg method. |
||
match instruction { | ||
Instruction::ArraySet { array, index, value, .. } => { | ||
if let Some(constant) = dfg.get_numeric_constant(*index) { | ||
if constant == target_index { | ||
return SimplifyResult::SimplifiedTo(*value); | ||
} | ||
|
||
array_id = *array; // recur | ||
} else { | ||
return SimplifyResult::None; | ||
} | ||
Instruction::MakeArray { elements: array, typ: _ } => { | ||
elements = Some(array.clone()); | ||
break; | ||
} | ||
_ => return SimplifyResult::None, | ||
} | ||
Instruction::MakeArray { elements: array, typ: _ } => { | ||
elements = Some(array.clone()); | ||
break; | ||
} | ||
_ => return SimplifyResult::None, | ||
} | ||
_ => return SimplifyResult::None, | ||
} else { | ||
return SimplifyResult::None; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,10 @@ | |
//! with a non-literal target can be replaced with a call to an apply function. | ||
//! The apply function is a dispatch function that takes the function id as a parameter | ||
//! and dispatches to the correct target. | ||
use std::collections::{BTreeMap, BTreeSet, HashSet}; | ||
use std::{ | ||
collections::{BTreeMap, BTreeSet, HashSet}, | ||
sync::Arc, | ||
}; | ||
|
||
use acvm::FieldElement; | ||
use iter_extended::vecmap; | ||
|
@@ -13,6 +16,7 @@ use crate::ssa::{ | |
function_builder::FunctionBuilder, | ||
ir::{ | ||
basic_block::BasicBlockId, | ||
dfg::GlobalsGraph, | ||
function::{Function, FunctionId, Signature}, | ||
instruction::{BinaryOp, Instruction}, | ||
types::{NumericType, Type}, | ||
|
@@ -278,8 +282,10 @@ fn create_apply_function( | |
function_ids: Vec<FunctionId>, | ||
) -> FunctionId { | ||
assert!(!function_ids.is_empty()); | ||
ssa.add_fn(|id| { | ||
let globals_dfg = std::mem::take(&mut ssa.globals.dfg); | ||
let new_id = ssa.add_fn(|id| { | ||
let mut function_builder = FunctionBuilder::new("apply".to_string(), id); | ||
function_builder.set_globals(Arc::new(GlobalsGraph::from_dfg(globals_dfg.clone()))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why we do the whole There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was originally due to |
||
let target_id = function_builder.add_parameter(Type::field()); | ||
let params_ids = vecmap(signature.params, |typ| function_builder.add_parameter(typ)); | ||
|
||
|
@@ -334,7 +340,9 @@ fn create_apply_function( | |
} | ||
} | ||
function_builder.current_function | ||
}) | ||
}); | ||
ssa.globals.dfg = globals_dfg; | ||
new_id | ||
} | ||
|
||
/// Crates a return block, if no previous return exists, it will create a final return | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit odd when
self.is_global
just matches on the value for aValue::Global
, I think it'd be clearer to just have the Global match case below return true instead of unreachableThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to separately check as I would need to special case
Value::Instruction
. I can switch to checking inside the match cases though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have switched it