Skip to content
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

feat(perf): Cache commutative instructions in constant folding #5832

Closed
wants to merge 3 commits into from

Conversation

vezenovm
Copy link
Contributor

Description

Problem*

Resolves

Summary*

Keeping a draft as I want to see how it runs in the code size benchmarks

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

Comment on lines 289 to 301
if let Instruction::Binary(binary) = &instruction {
match binary.operator {
BinaryOp::Add
| BinaryOp::Mul
| BinaryOp::Eq
| BinaryOp::And
| BinaryOp::Or
| BinaryOp::Xor => {
let commutative_instruction = Instruction::Binary(Binary {
lhs: binary.rhs,
rhs: binary.lhs,
operator: binary.operator,
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this hard check here, we should sort the operands when creating a commutative binary op. E.g. lower ValueIds are always the lhs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. However, we seem to be getting similar results as before.

Copy link
Contributor

github-actions bot commented Aug 27, 2024

Changes to Brillig bytecode sizes

Generated at commit: 6605711922cbb8ee068b8cdbbd9b00c869b47fcf, compared to commit: 3c778b73d9458ab708df21c850468d708676cde4

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
binary_operator_overloading -2 ✅ -0.44%

Full diff report 👇
Program Brillig opcodes (+/-) %
binary_operator_overloading 450 (-2) -0.44%

Copy link
Contributor

github-actions bot commented Aug 27, 2024

Changes to circuit sizes

Generated at commit: 6605711922cbb8ee068b8cdbbd9b00c869b47fcf, compared to commit: 3c778b73d9458ab708df21c850468d708676cde4

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
regression_5252 +72 ❌ +0.09% +72 ❌ +0.07%
slices +1 ❌ +0.32% +1 ❌ +0.03%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
regression_5252 77,070 (+72) +0.09% 97,311 (+72) +0.07%
slices 312 (+1) +0.32% 3,381 (+1) +0.03%
hashmap 200,183 (+10) +0.00% 361,213 (+10) +0.00%
conditional_1 4,784 (-3) -0.06% 38,799 (0) 0.00%
merkle_insert 1,780 (+1) +0.06% 29,032 (0) 0.00%
slice_dynamic_index 1,201 (+1) +0.08% 6,420 (0) 0.00%

@jfecher
Copy link
Contributor

jfecher commented Aug 27, 2024

I don't understand the regression_5252 size increase

@vezenovm
Copy link
Contributor Author

I don't understand the regression_5252 size increase

Yeah I don't either. I guess we can close this PR as it doesn't seem to be providing many benefits.

Comment on lines +198 to +200
// Sort operands of commutative instructions
if let Instruction::Binary(binary) = &instruction {
match binary.operator {
Copy link
Contributor

@jfecher jfecher Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I meant sorting when the instructions are first created. Not during constant folding. Probably won't remove the size increase in the tests though.

@vezenovm
Copy link
Contributor Author

Going to close this PR as the benefits seem minimal in Brillig code size and actually cause regressions for ACIR.

@vezenovm vezenovm closed this Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants