feat(ssa): Deduplicate intrinsics with predicates #6615
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 👇
|
Changes to circuit sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
| Intrinsic::AssertConstant | ||
| | Intrinsic::StaticAssert | ||
| | Intrinsic::ApplyRangeConstraint | ||
| | Intrinsic::AsWitness => deduplicate_with_predicate, |
There was a problem hiding this comment.
Is there a difference between these four and the previous ones?
There was a problem hiding this comment.
The comment above // As long as the constraints are applied on the same inputs. this case also feels implied by "can_be_deduplicated"
There was a problem hiding this comment.
The difference is in how much of the comments on the previous two don't apply on these. Basically followed the similar existing division in has_side_effects above. The ones in the first group are the ones I found appear in handle_instruction_side_effects in flatten_cfg, so these are tied to the predicate by the virtue of having their input being multiplied with it.
These others down here seemed related simply to applying assertions and constraints, which I assume work similar in that if you do the same constraint multiple times on the same thing then you can deduplicate. I hope this is true for AsWitness as well. I agree the comment above doesn't add any insight, I'll remove it.
There was a problem hiding this comment.
I am not sure if we want to deduplicate AsWitness. It is telling our ACIR gen to generate a new witness variable, which isn't exactly the same as an assertion. cc @TomAFrench
There was a problem hiding this comment.
Yes, I thought it might be enough to produce one witness for the same inputs, and hoped to clarify in the PR review with y’all
There was a problem hiding this comment.
I think for the purposes of std::as_witness, it doesn't require that distinct witnesses are created so I'd deduplicate these.
| Intrinsic::AssertConstant | ||
| | Intrinsic::StaticAssert | ||
| | Intrinsic::ApplyRangeConstraint | ||
| | Intrinsic::AsWitness => deduplicate_with_predicate, |
There was a problem hiding this comment.
The comment above // As long as the constraints are applied on the same inputs. this case also feels implied by "can_be_deduplicated"
Description
Problem*
Resolves #6533
Summary*
Allows
Instruction::Intrinsictypes to be deduplicated inconstant_foldingif theContextis inuse_constraint_infomode, and theIntrinsicis one that doesn't have a side effect other than the possibility of violating an implicit constraint on its inputs, which should be preserved in the first occurrence.Additional Context
I found it a bit confusing why we're using
use_constraint_infofor these whenrequires_acir_gen_predicatereturnsfalsefor all these instructions, which will causecached_instruction_resultsto containNonefor the predicate regardless ofuse_constraint_info. For some, but not all intrinsics, the predicate that tells whether the side effect is enabled is attached to their inputs duringSsa::flatten_cfg, and they don't depend any more on theenable_side_effectinstruction. But it seems like this flag is the one that communicates whether we want such instructions to be deduplicated, so I used it even though I'm not sure what adverse effect we would see if we just returnedtruefor these, other than the implicit coupling by knowing thatflatten_cfghas done things in a certain way.Another method that seems to have some of the truth is
remove_enable_side_effects::Context::responds_to_side_effects_varDocumentation*
Check one:
PR Checklist*
cargo fmton default settings.