feat(SSA): replace slice intrinsics returned length with constant#10301
feat(SSA): replace slice intrinsics returned length with constant#10301
Conversation
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
|
The optimizations here aren't that big. However, if we implement #10296 then it will happen that intrinsics that were optimized out will now be fully optimized (as the slice contents won't be needed, and neither the length) so DIE will remove them, resulting in (I think) less ACIR/Brillig. But, in any case, this is a relatively simple optimization that we could merge as-is. |
jfecher
left a comment
jfecher
left a comment
There was a problem hiding this comment.
I'm thinking some larger changes to the slice methods may actually be better than a new pass
| /// 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`. |
There was a problem hiding this comment.
The fact this is always known makes me question whether we should have slice_insert return 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 pop on an empty array. We could still do a new_len = if len == 0 { 0 } else { len - 1 } though.
There was a problem hiding this comment.
Ah, it's not always known. For example in slice_dynamic_index I can see this:
v812, v813, v814 = call slice_pop_back(v789, v811) -> (u32, [Field], Field)
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.
On the other hand, if we replace v812 above with v812 = add v789, u32 1 then 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.
Ah, it's not always known. For example in slice_dynamic_index I can see this:
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.
![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 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: ade36c5 | Previous: b43153b | Ratio |
|---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
3 s |
2 s |
1.50 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
|
Closing until #10206 is merged. |
2 participants
Description
Problem
Resolves #9917
Summary
Additional Context
Documentation
Check one:
PR Checklist
cargo fmton default settings.