-
Notifications
You must be signed in to change notification settings - Fork 231
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
Increase in memory used to compile rollup-base-public
#7001
Comments
I think a contributing factor this is how early we perform the inlining pass. Looking at the ordering of passes: noir/compiler/noirc_evaluator/src/ssa.rs Lines 153 to 191 in 74d258f
You can see that we pretty much immediately inline all of the functions into the entrypoint function. This means that if we've got a function which is used in multiple places, we're going to have to do all the later passes N different times rather than fully simplifying the function on its own before we inline it. Note that we can't just run all the various passes on every function before inlining. We're going to need to at the least tolerate loop unrolling failing as loop bounds may come from function arguments, also some thought would need to go into flattening as well (maybe?) |
The various maps in Edit: perhaps an easier change would be to add a check to drop any blocks we don't need any more (blocks whose successors are all finished as well). |
A bit more digging shows that there is a cambrian explosion in the number of blocks during the
After Unrolling the number of blocks in the
EDIT: Only after this did I separate
|
I'm not seeing simplification creating lots of blocks, it seems like they exist at the beginning of that pass so they're most likely from the loop unrolling pass. tbh, the more I look at this the more it seems like anything but switching to a bottom-up approach to inlining is pointless. Consider the function below.... This is an implementation of the This whole stack of SSA will eventually become some casts and a single
|
My thoughts are that we should build a callgraph of all the functions in the program, as a first pass at this we can then:
|
I don't disagree with anything you said, I also thought that functions that end up being called could be massaged a little bit before inlining, although they appear multiple times because of the different parameters they are called with. For the record the number of blocks in functions was printed like this: fn run_pass<F>(mut self, pass: F, msg: &str) -> Self
where
F: FnOnce(Ssa) -> Ssa,
{
self.ssa = time(msg, self.print_codegen_timings, || pass(self.ssa));
println!("AFTER {msg}: functions={}", self.ssa.functions.len());
for f in self.ssa.functions.values() {
let block_cnt = PostOrder::with_function(f).into_vec().len();
println!(" FUNCTION {}: blocks={block_cnt}", f.name());
}
self.print(msg)
} If the increase in the number blocks isn't coming from the pass it says then I'm not sure what could cause them. There is no concurrency here to mix up the print order. |
I'll have a look at replicating this as if we're getting a blowup in blocks within the simplification pass then I'm missing something important. |
As noticed by @TomAFrench , the above count did not cover
|
Hah, got the -50% on |
Aim
Followup for #6972 added a new
mem2reg
pass. As a consequence we saw a 100% increase in memory used during the compilation of one of the protocol circuits in aztec-packages.Expected Behavior
Didn't expect a significant increase in memory usage.
Bug
#6972 (comment)
To Reproduce
See how CI does it.
Workaround
None
Workaround Description
No response
Additional Context
No response
Project Impact
None
Blocker Context
No response
Nargo Version
nargo version = 1.0.0-beta.1 noirc version = 1.0.0-beta.1+bb8dd5ce43f0d89e393bd49f8415008826903652 (git version hash: 13b5871, is dirty: false)
NoirJS Version
No response
Proving Backend Tooling & Version
No response
Would you like to submit a PR for this Issue?
None
Support Needs
No response
The text was updated successfully, but these errors were encountered: