fix: don't deduplicate constraints in blocks that are not dominated #6628
Conversation
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 👇
|
| ) -> bool { | ||
| // These two can never be hoisted as they have a side-effect | ||
| // (though it's fine to de-duplicate them, just not fine to hoist them) | ||
| if matches!(instruction, Instruction::Constrain(..) | Instruction::RangeCheck { .. }) { |
There was a problem hiding this comment.
Wouldn't it be enough to do this for Constrain, because that's the one that interacts with simplifications that can replace other instructions' results? Other instructions can have side effects too, but Constrain has special role in constant_folding. I don't see RangeCheck have the same.
There was a problem hiding this comment.
I think any instruction can be simplified. At least instruction here comes from resolve_instruction where any Instruction type can be returned.
There was a problem hiding this comment.
What I meant was that we only have simplifications that come from Constrain, and I thought the hoisting of Constrain in the dominator is what's causing trouble further on. Just not sure what RangeCheck would do.
|
I'd like to avoid having 2 PRs with 99% of the same code. Especially as #6627 is getting refactorings which disguise the meaningful differences in approach being taken. Let's park this PR and get the other one merged to fix the correctness bug. We can then revisit how we can maintain more optimizations. |
|
Sounds good. The other PR also has more refactors and the code is a bit more readable 👍 |
Description
Problem
Resolves #6588
Summary
Alternative to the approach in #6627. Here we keep track of all the simplifications a value could have (one per block) while #6627 only kept one simplification... which works, but this PR should result in more simplifications 🤞
Additional Context
There are a lot of nested structures in this file and it would be nice to somehow refactor it... but maybe it can be done in a separate PR, now that we have a regression test for the bug.
Documentation
Check one:
PR Checklist
cargo fmton default settings.