feat(ssa): Treat globals as constant in a function's DFG #7040
Conversation
Co-authored-by: jfecher <[email protected]>
Compilation Memory Report
|
Execution Memory Report
|
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Execution Report
|
Compilation Report
|
For some reason it seems inlining global constants immediately causes this test to regress. We saw the same improvement as this regression in #6985 (comment). |
…' into mv/treat-globals-as-const-in-dfg
| if self.is_global(argument) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This is a bit odd when self.is_global just matches on the value for a Value::Global, I think it'd be clearer to just have the Global match case below return true instead of unreachable
There was a problem hiding this comment.
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.
I have switched it
| pub(crate) fn get_instruction(&self, value: ValueId) -> Option<&Instruction> { | ||
| match &self[value] { |
There was a problem hiding this comment.
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 self.values[value] directly. It's probably fine then but something to keep in mind now.
There was a problem hiding this comment.
I'll add a safety comment either way.
How does the name get_local_or_global_instruction sound?
There was a problem hiding this comment.
I made the rename to get_local_or_global_instruction with an assertion for MakeArray when we have a global instruction. I also included a small comment stating that this may come from a separate DFG.
| let value = &self.values[id]; | ||
| if matches!(value, Value::Global(_)) { | ||
| return &self.globals[id]; | ||
| } | ||
| value |
There was a problem hiding this comment.
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.
For arrays of integers, all those integers should be Value::Globals as well, right?
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.
| array_id = *array; // recur | ||
| } else { | ||
| return SimplifyResult::None; | ||
| if let Some(instruction) = dfg.get_instruction(array_id) { |
There was a problem hiding this comment.
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.
It is needed for the MakeArray case not the ArraySet case. I could assert inside of the DFG method that the instruction must be MakeArray for safety.
There was a problem hiding this comment.
I added an insertion inside of the dfg method.
| 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.
I don't understand why we do the whole std::mem::take thing with the globals dfg here. Shouldn't we have a Arc<GlobalsGraph> somewhere? If Ssa doesn't already store a Arc<GlobalsGraph> then it probably should - we should avoid cloning entire DFGs if possible - even small ones like globals is likely expected to be.
There was a problem hiding this comment.
It was originally due to add_fn mutably borrowing ssa. I switched to using function_ids to access a function's globals graph. This is safe due to assert!(!function_ids.is_empty());.
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Description
Problem*
Resolves TODO leftover from #6985
Also mentioned in this comment #7021 (comment).
Summary*
In order to re-use our existing instruction simplification we need to be able to access globals inside of the DFG. In order to do this I have created a new
GlobalsGraphtype that now exists as anArc<GlobalsGraph>in theDataFlowGraph.Arcis used so that we can share theGlobalsGraphacross all functions. However, we will have to eat a cloning cost as we now place the globals information in all functions rather than simply placing it in the mainSsaobject.Additional Context
In #6985 we only added a
Value::Global, but globals support both numeric constants and array constants. This means that we can have overlapping instruction ids in the function graph and the globals graph. To avoid having to add placeholder instructions like we do for values, when checking for a constant array instruction we also check whether the value tied to that instruction is specified as a global. If the instruction is specified as global we look for the instruction in the globals graph.Documentation*
Check one:
PR Checklist*
cargo fmton default settings.