-
Notifications
You must be signed in to change notification settings - Fork 387
feat(SSA): replace slice intrinsics returned length with constant #10301
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
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
265 changes: 265 additions & 0 deletions
265
compiler/noirc_evaluator/src/ssa/opt/slice_intrinsics_length.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,265 @@ | ||
| use acvm::{AcirField as _, FieldElement}; | ||
|
|
||
| use crate::ssa::{ | ||
| ir::{ | ||
| function::Function, | ||
| instruction::{Instruction, Intrinsic}, | ||
| types::NumericType, | ||
| }, | ||
| ssa_gen::Ssa, | ||
| }; | ||
|
|
||
| impl Ssa { | ||
| /// A simple SSA optimization that replaces length values returned from slice intrinsics | ||
| /// with known constants if the input length to those intrinsics are constants. | ||
| /// | ||
| /// For example, if we have: | ||
| /// | ||
| /// ```ssa | ||
| /// v1, v2 = slice_insert(u32 10, v0, u32 5, Field 42) -> (u32, [Field]) | ||
| /// ``` | ||
| /// | ||
| /// where `v1` is the returned length, we can replace `v1` with `u32 11` since we know | ||
| /// the returned length will be one more than the input length `u32 10`. | ||
| #[tracing::instrument(level = "trace", skip(self))] | ||
| pub(crate) fn slice_intrinsics_length_optimization(mut self) -> Self { | ||
| for func in self.functions.values_mut() { | ||
| func.slice_intrinsics_length_optimization(); | ||
| } | ||
| self | ||
| } | ||
| } | ||
|
|
||
| impl Function { | ||
| pub(crate) fn slice_intrinsics_length_optimization(&mut self) { | ||
| let slice_insert = self.dfg.get_intrinsic(Intrinsic::SliceInsert).copied(); | ||
| let slice_remove = self.dfg.get_intrinsic(Intrinsic::SliceRemove).copied(); | ||
| let slice_push_back = self.dfg.get_intrinsic(Intrinsic::SlicePushBack).copied(); | ||
| let slice_push_front = self.dfg.get_intrinsic(Intrinsic::SlicePushFront).copied(); | ||
| let slice_pop_back = self.dfg.get_intrinsic(Intrinsic::SlicePopBack).copied(); | ||
| let slice_pop_front = self.dfg.get_intrinsic(Intrinsic::SlicePopFront).copied(); | ||
|
|
||
| let ops = [ | ||
| slice_insert, | ||
| slice_remove, | ||
| slice_push_back, | ||
| slice_push_front, | ||
| slice_pop_back, | ||
| slice_pop_front, | ||
| ]; | ||
| if ops.iter().all(Option::is_none) { | ||
| // No slice intrinsics used in this function | ||
| return; | ||
| } | ||
|
|
||
| self.simple_optimization(|context| { | ||
| let instruction_id = context.instruction_id; | ||
| let instruction = context.instruction(); | ||
|
|
||
| let (target_func, arguments) = match &instruction { | ||
| Instruction::Call { func, arguments } => (func, arguments), | ||
| _ => return, | ||
| }; | ||
|
|
||
| let replacement = if slice_insert.is_some_and(|op| target_func == &op) | ||
| || slice_push_front.is_some_and(|op| target_func == &op) | ||
| || slice_push_back.is_some_and(|op| target_func == &op) | ||
| { | ||
| context.dfg.get_numeric_constant(arguments[0]).map(|length| { | ||
| // For `slice_insert(length, ...)` we can replace the resulting length with length + 1. | ||
| // Same goes for `slice_push_front` and `slice_push_back`. | ||
| let length = length + FieldElement::one(); | ||
| let new_slice_length = context.dfg.instruction_results(instruction_id)[0]; | ||
| (new_slice_length, length) | ||
| }) | ||
| } else if slice_remove.is_some_and(|op| target_func == &op) | ||
| || slice_pop_back.is_some_and(|op| target_func == &op) | ||
| { | ||
| context.dfg.get_numeric_constant(arguments[0]).and_then(|length| { | ||
| if !length.is_zero() { | ||
| // For `slice_remove(length, ...)` we can replace the resulting length with length - 1. | ||
| // Same goes for `slice_pop_back`. | ||
| let length = length - FieldElement::one(); | ||
| let new_slice_length = context.dfg.instruction_results(instruction_id)[0]; | ||
| Some((new_slice_length, length)) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| } else if slice_pop_front.is_some_and(|op| target_func == &op) { | ||
| context.dfg.get_numeric_constant(arguments[0]).and_then(|length| { | ||
| if !length.is_zero() { | ||
| // For `slice_pop_front(length, ...)` we can replace the resulting length with length - 1. | ||
| let length = length - FieldElement::one(); | ||
| // Note that `(popped_element, new_slice)` is returned so the new length is | ||
| // the before last result. | ||
| let results = context.dfg.instruction_results(instruction_id); | ||
| let new_slice_length = results[results.len() - 2]; | ||
| Some((new_slice_length, length)) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| if let Some((value_to_replace, replacement)) = replacement { | ||
| let known_length = | ||
| context.dfg.make_constant(replacement, NumericType::length_type()); | ||
| context.replace_value(value_to_replace, known_length); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test { | ||
| use crate::assert_ssa_snapshot; | ||
|
|
||
| use super::Ssa; | ||
|
|
||
| #[test] | ||
| fn slice_insert_optimization() { | ||
| let src = " | ||
| acir(inline) fn main f0 { | ||
| b0(): | ||
| v0 = make_array [Field 2, Field 3] : [Field] | ||
| v1, v2 = call slice_insert(u32 2, v0, u32 1, Field 4) -> (u32, [Field]) | ||
| return v1 | ||
| } | ||
| "; | ||
| let ssa = Ssa::from_str(src).unwrap(); | ||
|
|
||
| // Here `v1` was replaced with 3 because we know the new length is 2 + 1 | ||
| let ssa = ssa.slice_intrinsics_length_optimization(); | ||
| assert_ssa_snapshot!(ssa, @r" | ||
| acir(inline) fn main f0 { | ||
| b0(): | ||
| v2 = make_array [Field 2, Field 3] : [Field] | ||
| v7, v8 = call slice_insert(u32 2, v2, u32 1, Field 4) -> (u32, [Field]) | ||
| return u32 3 | ||
| } | ||
| "); | ||
| } | ||
|
|
||
| #[test] | ||
| fn slice_remove_optimization() { | ||
| let src = " | ||
| acir(inline) fn main f0 { | ||
| b0(): | ||
| v0 = make_array [Field 2, Field 3] : [Field] | ||
| v1, v2, v3 = call slice_remove(u32 2, v0, u32 1) -> (u32, [Field], Field) | ||
| return v1 | ||
| } | ||
| "; | ||
| let ssa = Ssa::from_str(src).unwrap(); | ||
|
|
||
| // Here `v1` was replaced with 1 because we know the new length is 2 - 1 | ||
| let ssa = ssa.slice_intrinsics_length_optimization(); | ||
| assert_ssa_snapshot!(ssa, @r" | ||
| acir(inline) fn main f0 { | ||
| b0(): | ||
| v2 = make_array [Field 2, Field 3] : [Field] | ||
| v6, v7, v8 = call slice_remove(u32 2, v2, u32 1) -> (u32, [Field], Field) | ||
| return u32 1 | ||
| } | ||
| "); | ||
| } | ||
|
|
||
| #[test] | ||
| fn slice_push_front_optimization() { | ||
| let src = " | ||
| acir(inline) fn main f0 { | ||
| b0(): | ||
| v0 = make_array [Field 2, Field 3] : [Field] | ||
| v1, v2 = call slice_push_front(u32 2, v0, Field 4) -> (u32, [Field]) | ||
| return v1 | ||
| } | ||
| "; | ||
| let ssa = Ssa::from_str(src).unwrap(); | ||
|
|
||
| // Here `v1` was replaced with 1 because we know the new length is 2 + 1 | ||
| let ssa = ssa.slice_intrinsics_length_optimization(); | ||
| assert_ssa_snapshot!(ssa, @r" | ||
| acir(inline) fn main f0 { | ||
| b0(): | ||
| v2 = make_array [Field 2, Field 3] : [Field] | ||
| v6, v7 = call slice_push_front(u32 2, v2, Field 4) -> (u32, [Field]) | ||
| return u32 3 | ||
| } | ||
| "); | ||
| } | ||
|
|
||
| #[test] | ||
| fn slice_push_back_optimization() { | ||
| let src = " | ||
| acir(inline) fn main f0 { | ||
| b0(): | ||
| v0 = make_array [Field 2, Field 3] : [Field] | ||
| v1, v2 = call slice_push_back(u32 2, v0, Field 4) -> (u32, [Field]) | ||
| return v1 | ||
| } | ||
| "; | ||
| let ssa = Ssa::from_str(src).unwrap(); | ||
|
|
||
| // Here `v1` was replaced with 1 because we know the new length is 2 + 1 | ||
| let ssa = ssa.slice_intrinsics_length_optimization(); | ||
| assert_ssa_snapshot!(ssa, @r" | ||
| acir(inline) fn main f0 { | ||
| b0(): | ||
| v2 = make_array [Field 2, Field 3] : [Field] | ||
| v6, v7 = call slice_push_back(u32 2, v2, Field 4) -> (u32, [Field]) | ||
| return u32 3 | ||
| } | ||
| "); | ||
| } | ||
|
|
||
| #[test] | ||
| fn slice_pop_back_optimization() { | ||
| let src = " | ||
| acir(inline) fn main f0 { | ||
| b0(): | ||
| v0 = make_array [Field 2, Field 3] : [Field] | ||
| v1, v2, v3 = call slice_pop_back(u32 2, v0) -> (u32, [Field], Field) | ||
| return v1 | ||
| } | ||
| "; | ||
| let ssa = Ssa::from_str(src).unwrap(); | ||
|
|
||
| // Here `v1` was replaced with 1 because we know the new length is 2 - 1 | ||
| let ssa = ssa.slice_intrinsics_length_optimization(); | ||
| assert_ssa_snapshot!(ssa, @r" | ||
| acir(inline) fn main f0 { | ||
| b0(): | ||
| v2 = make_array [Field 2, Field 3] : [Field] | ||
| v5, v6, v7 = call slice_pop_back(u32 2, v2) -> (u32, [Field], Field) | ||
| return u32 1 | ||
| } | ||
| "); | ||
| } | ||
|
|
||
| #[test] | ||
| fn slice_pop_front_optimization() { | ||
| let src = " | ||
| acir(inline) fn main f0 { | ||
| b0(): | ||
| v0 = make_array [Field 2, Field 3] : [Field] | ||
| v1, v2, v3 = call slice_pop_front(u32 2, v0) -> (Field, u32, [Field]) | ||
| return v2 | ||
| } | ||
| "; | ||
| let ssa = Ssa::from_str(src).unwrap(); | ||
|
|
||
| // Here `v2` was replaced with 1 because we know the new length is 2 - 1 | ||
| let ssa = ssa.slice_intrinsics_length_optimization(); | ||
| assert_ssa_snapshot!(ssa, @r" | ||
| acir(inline) fn main f0 { | ||
| b0(): | ||
| v2 = make_array [Field 2, Field 3] : [Field] | ||
| v5, v6, v7 = call slice_pop_front(u32 2, v2) -> (Field, u32, [Field]) | ||
| return u32 1 | ||
| } | ||
| "); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The fact this is always known makes me question whether we should have
slice_insertreturn two values at all.Perhaps slice_insert, slice_push_front, and friends should instead return just the array, and when handling them we should insert a separate add 1 instruction for the length. That way we need less special handling. The only time insert/remove wont change the length is if the index is OOB, in which case we should halt anyway. The only case the length doesn't change on a non-error is when we call
popon an empty array. We could still do anew_len = if len == 0 { 0 } else { len - 1 }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.
Ah, it's not always known. For example in
slice_dynamic_indexI can see this:That said, I agree that maybe the length doesn't need to be returned and it can be computed from the input. I think in ACIR that's what we do (I didn't check Brillig). But I don't know how this can be changed if the Noir function does return the length (in the slice return type).
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.
On the other hand, if we replace
v812above withv812 = add v789, u32 1then it's one more operation that needs to be done in ACIR/Brillig for dynamic indexes, so maybe it'll add some overhead.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.
Not sure what you mean - looks like the length would still be known to be 1 more than the previous. By unknown I meant unknown whether it'd increment at all or not, not whether the result is unknown.
Would add/sub be one more operation that need to be handled for dynamic indices? I would think we'd need to already handle that case since indices can come from addition/subtraction already. If the analysis only handles
slice_functions then we could even change it not to handle them at all anymore since now the lengths wouldn't be coming from those operations but from separate add/subs.