chore: let Function::inlined take a should_inline_call function
#7149
Conversation
|
If we're marking this as resolving #7148, can you break out issues for all the extra inlining conditions it specifies? |
|
I changed it to be "part of". Regarding the extra inlining conditions, are those to be done in |
|
(if yes, I can try to implement those in this PR, or at least the first two which seem simpler than the last one) |
| if called_func_id == self.id() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Note that these checks should be enforced even if we're passing a different inlining condition in as the closure as theyre important safety checks. Can you pull them out of this function and ensure they're always enforced?
| if self.runtime().is_acir() { | ||
| // We never inline a brillig function into an ACIR function. | ||
| return false; |
There was a problem hiding this comment.
ditto here and for inlining ACIR into brillig.
There was a problem hiding this comment.
and for inlining ACIR into brillig
Is that a new check that should be added?
| } | ||
|
|
||
| /// Generic function that determines whether a function should inline a call. | ||
| pub(super) fn should_inline_call( |
There was a problem hiding this comment.
We don't want to enshrine this as the only way of determining whether a function call should be inlined or not (quite the opposite) so I don't think it should be defined as a method on Function.
This should be able to be calculated using |
I don't think so as |
|
I addressed all the comments (I think). To avoid a double lookup in |
| match callee.runtime() { | ||
| RuntimeType::Acir(inline_type) => { | ||
| // If the called function is acir, we inline if it's not an entry point | ||
| if inline_type.is_entry_point() { | ||
| return None; | ||
| } | ||
| } | ||
| RuntimeType::Brillig(_) => { | ||
| if self.source_function.runtime().is_acir() { | ||
| // We never inline a brillig function into an ACIR function. | ||
| return None; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Now that we seperate the runtimes at SSAgen time we can simplify this to enforcing that the caller and callee runtimes are the same (up to the inlining type).
There was a problem hiding this comment.
Done! I couldn't easily check that the two runtime types match (because of InlineType) so I compared the bools of is_acir().
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Execution Memory'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: f8ab025 | Previous: 7f9525d | Ratio |
|---|---|---|---|
rollup-block-root |
4280 MB |
1250 MB |
3.42 |
rollup-base-public |
1180 MB |
730.82 MB |
1.61 |
rollup-base-private |
803.26 MB |
591.22 MB |
1.36 |
private-kernel-reset |
346.48 MB |
245.61 MB |
1.41 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
|
There's clearly something wrong but I can't tell what. I'll continue debugging this tomorrow. |
Description
Problem
Part of #7148
Summary
Additional Context
Documentation
Check one:
PR Checklist
cargo fmton default settings.