fix: defunctionalize pass on the caller runtime to apply #7100
+214
−50
Conversation
This file contains 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
Execution Report
|
Compilation Report
|
5 tasks
Compilation Memory Report
|
Execution Memory Report
|
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
|
Pushed a small change to signal that we want to inline these functions more aggressively. The inliner isn't picking this up currently. |
|
That's a good idea, I was also wondering about it: the cost will always be above 0 by the looks of it, yet it seems like something that works better when inlined. This is where |
|
Nah, I looked at adding a line to the closure which determine whether to inline to force it to inline |
|
I think the fn main(w: Field) -> pub Field {
let ret = twice(add1, 3);
test_array_functions();
w + ret
}
/// Test the array functions in std::array
fn test_array_functions() {
let two = 2; // giving this a name, to ensure that the Option functions work with closures
let myarray: [i32; 3] = [1, 2, 3];
assert(myarray.any(|n| n > 2));
assert(myarray.any(|n| n > two));
let evens: [i32; 3] = myarray.map(|n| n * two); // [2, 4, 6]
assert(evens.all(|n| n > 1));
assert(evens.all(|n| n >= two));
assert(evens.fold(0, |a, b| a + b) == 12);
assert(evens.fold(0, |a, b| a + b + two) == 18);
assert(evens.reduce(|a, b| a + b) == 12);
assert(evens.reduce(|a, b| a + b + two) == 16);
assert(evens.map(|n| n / 2) == myarray);
assert(evens.map(|n| n / two) == myarray);
}
fn add1(x: Field) -> Field {
x + 1
}
fn twice(f: fn(Field) -> Field, x: Field) -> Field {
f(f(x))
}Up and including the Loop Invariant Code Motion pass the difference between these calls is only in the inclusion of Then, after unrolling, in ACIR we are left with 13 blocks, vs 33 with |
|
Indeed if I don't skip unrolling here then the number of opcodes go down from 641 to 23 |
|
Here are the loop statistics, used to decide if they are considered small or not: We have a check to limit byte code growth percentage due to unrolling, but it's not set to anything by default. This I tried cheating by considering any loop with up to 15 instructions as small, it still resulted in 475 instructions. Only by unrolling all of them did it collapse down. Unfortunately just unrolling |
10 tasks
michaeljklein
requested changes
Jan 17, 2025
|
On
|
michaeljklein
approved these changes
Jan 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Problem*
Extracted from #7072
Summary*
Updates
defunctionalizeto create theapplyfunction with theruntimeinherited from its caller.If it's called from both ACIR and Brillig then two functions are created. This is because at this point the functions have already been monomorphised, and by having two different versions we can apply different rules. Or at least it keeps the logic simpler: if we use
--force-brilligthen we don't wantacirfunctions to pop up; if we don't, then I suppose there wouldn't be any harm callingacirfrombrilligas it would be inlined.Another potential issue of not separating the runtime would be if for example an acir-only instruction was added to
apply, and then it would be inlined into a brillig function, causing a panic later; although I'm not sure how this is avoided now (can't brillig functions still call ACIR ones and inline their stuff?).TODO:
See this and this comment about the increase in size of certain tests.
Additional Context
The problem with not inheriting the runtime was that it interfered with inlining: any Brillig function called from ACIR was considered an entry point and wasn't inlined into anything. OTOH the ACIR
applywas inlined into its Brillig caller, which resulted in it being removed in the next inline pass, and then the original Brillig functions could be inlined in a third inline pass. Altogether confusing, and fixed in #7072Documentation*
Check one:
PR Checklist*
cargo fmton default settings.