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: add initial optimizations for multi-instruction arithmetic #5709

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

TomAFrench
Copy link
Member

Description

Problem*

Resolves (partially) #5707

Summary*

This PR add some simple handling for simplifying arithmetic which spans multiple instructions. I've only handled a couple of simple cases and made some (imo reasonable) assumptions about the shape of the SSA instructions being optimised in order to keep the code simple.

Additional Context

One thing to consider with these forms of optimisations is that we can end up removing range checks which would have been applied to intermediate values, eg.

fn main(x: u8) {
    let x2 = x + 255 - 255;
    assert_eq(x, x2);
}

This program would originally produce the ACIR

Compiled ACIR for main (unoptimized):
func 0
current witness index : 2
private parameters indices : [0]
public parameters indices : []
return value indices : []
BLACKBOX::RANGE [(0)] [ ]
EXPR [ (1, _0) (-1, _1) 255 ]
BLACKBOX::RANGE [(1)] [ ]
EXPR [ (1, _1) (-1, _2) -255 ]
BLACKBOX::RANGE [(2)] [ ]
EXPR [ (1, _0) (-1, _2) 0 ]

which would obviously fail for any inputs other than x=0 whereas now this program will always succeed. I think it's desirable to "rearrange the users arithmetic" in such a way to avoid overflows on intermediate values however.

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.

Copy link
Contributor

github-actions bot commented Aug 10, 2024

Changes to circuit sizes

Generated at commit: 5ae07d4e603db21e9469751f7919357f07d16715, compared to commit: a1b50466bfd8c44d50440e00ecb50e29425471e5

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
references +40 ❌ +666.67% +37 ❌ +370.00%
closures_mut_ref +1 ❌ +100.00% +1 ❌ +16.67%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
references 46 (+40) +666.67% 47 (+37) +370.00%
closures_mut_ref 2 (+1) +100.00% 7 (+1) +16.67%
slice_dynamic_index 1,380 (+180) +15.00% 6,751 (+332) +5.17%
slices 981 (+123) +14.34% 4,035 (+161) +4.16%
7_function 261 (+27) +11.54% 3,017 (+25) +0.84%
array_dynamic_nested_blackbox_input 532 (-2) -0.37% 38,904 (0) 0.00%
regression_4449 26,325 (-142) -0.54% 306,619 (-160) -0.05%
nested_array_dynamic 4,151 (-8) -0.19% 13,745 (-11) -0.08%
nested_array_in_slice 1,053 (-4) -0.38% 5,590 (-6) -0.11%
hashmap 93,254 (-320) -0.34% 150,560 (-440) -0.29%

@TomAFrench
Copy link
Member Author

TomAFrench commented Aug 10, 2024

Regressions are cause by the case where an intermediate value is being constrained and this allows another instruction to be resolved to a constant, e.g.

acir(inline) fn main f0 {
  b0(v0: Field):
    v17 = add v0, Field 1
    v18 = eq v17, Field 1
    constrain v17 == Field 1
    v20 = add v17, Field 2
    v21 = eq v20, Field 3
    constrain v20 == Field 3
    return 
}

Now that we're rewriting v20 in terms of v0 we cannot remove the second constrain instruction.

@TomAFrench TomAFrench force-pushed the tf/mvp-multi-instruction-arithmetic branch from bb77a2c to 5a84fc5 Compare August 11, 2024 04:33
@TomAFrench TomAFrench force-pushed the tf/mvp-multi-instruction-arithmetic branch from 5a84fc5 to e79f246 Compare August 11, 2024 04:35
@TomAFrench
Copy link
Member Author

This is starting to fall into the same issues as the "bubble up" SSA pass.

@jfecher
Copy link
Contributor

jfecher commented Aug 12, 2024

Hmm the optimization clash with constant folding with constraints certainly makes this less of a simple issue

@TomAFrench
Copy link
Member Author

This may need to be switched into a dedicated SSA pass rather than something to be done immediately on inserting the instruction.

Copy link
Contributor

github-actions bot commented Oct 1, 2024

Changes to Brillig bytecode sizes

Generated at commit: 5ae07d4e603db21e9469751f7919357f07d16715, compared to commit: a1b50466bfd8c44d50440e00ecb50e29425471e5

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
closures_mut_ref +5 ❌ +35.71%
brillig_references -2 ✅ -13.33%
fold_distinct_return -5 ✅ -23.81%

Full diff report 👇
Program Brillig opcodes (+/-) %
closures_mut_ref 19 (+5) +35.71%
brillig_acir_as_brillig 26 (+3) +13.04%
brillig_calls 26 (+3) +13.04%
reference_only_used_as_alias 256 (+1) +0.39%
sha2_byte 3,148 (-1) -0.03%
7_function 565 (-2) -0.35%
array_dynamic_nested_blackbox_input 957 (-4) -0.42%
regression_struct_array_conditional 579 (-4) -0.69%
nested_array_in_slice 1,222 (-16) -1.29%
nested_array_dynamic 2,425 (-36) -1.46%
brillig_references 13 (-2) -13.33%
fold_distinct_return 16 (-5) -23.81%

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