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: simplify jmpifs by reversing branches if condition is negated #5891

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

TomAFrench
Copy link
Member

Description

Problem*

Resolves

Summary*

Currently if an if-else block has a negated condition we generate an extra instruction to calculate the condition rather than just swapping the "if" and "else" branches. I've added a check for this in the simplify_cfg pass.

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.

Copy link
Contributor

github-actions bot commented Sep 3, 2024

Changes to Brillig bytecode sizes

Generated at commit: 23deeb387938a697768c8ee112f0029e424a0f50, compared to commit: 2303615815a2a60de8ac3dd53349f85201660917

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
acir_inside_brillig_recursion -1 ✅ -0.94%
brillig_recursion -1 ✅ -1.09%
fold_fibonacci -1 ✅ -1.09%
brillig_slices -8 ✅ -1.74%
references -13 ✅ -4.87%

Full diff report 👇
Program Brillig opcodes (+/-) %
regression_4709 133,780 (-2) -0.00%
regression_5252 5,145 (-1) -0.02%
debug_logs 4,991 (-1) -0.02%
poseidonsponge_x5_254 4,318 (-1) -0.02%
u128 3,106 (-1) -0.03%
nested_array_dynamic 2,460 (-1) -0.04%
bigint 2,039 (-1) -0.05%
nested_array_in_slice 1,237 (-1) -0.08%
uhashmap 23,493 (-22) -0.09%
brillig_cow_regression 1,965 (-2) -0.10%
sha2_byte 3,145 (-4) -0.13%
slice_dynamic_index 2,571 (-4) -0.16%
sha256_regression 6,881 (-12) -0.17%
eddsa 11,157 (-22) -0.20%
array_dynamic_nested_blackbox_input 959 (-2) -0.21%
ecdsa_secp256k1 953 (-2) -0.21%
sha256_var_size_regression 1,898 (-4) -0.21%
aes128_encrypt 458 (-1) -0.22%
6 1,746 (-4) -0.23%
conditional_1 1,248 (-3) -0.24%
regression_4449 799 (-2) -0.25%
array_dynamic_blackbox_input 1,145 (-3) -0.26%
hashmap 26,303 (-69) -0.26%
brillig_sha256 740 (-2) -0.27%
array_sort 339 (-1) -0.29%
conditional_regression_short_circuit 1,317 (-4) -0.30%
sha256_var_witness_const_regression 1,270 (-4) -0.31%
sha256 1,855 (-6) -0.32%
slice_regex 2,422 (-17) -0.70%
slices 2,007 (-19) -0.94%
acir_inside_brillig_recursion 105 (-1) -0.94%
brillig_recursion 91 (-1) -1.09%
fold_fibonacci 91 (-1) -1.09%
brillig_slices 453 (-8) -1.74%
references 254 (-13) -4.87%

@TomAFrench
Copy link
Member Author

I'm really unsure on why exactly this is causing test failures. The ordering of the branches in a jmpif shouldn't cause any changes to behaviour afaik.

@TomAFrench
Copy link
Member Author

This branch changes the order of the jmpif as expected in the simplify_cfg pass however the issue comes up in flattening as we end up merging the values incorrectly.

Master

acir(inline) fn main f0 {
  b0(v0: Field):
    v1 = allocate
    v2 = allocate
    store Field 0 at v2
    v5 = eq v0, Field 10
    v6 = not v5
    enable_side_effects v6
    v7 = load v2
    store Field 2 at v2
    store v7 at v2
    enable_side_effects u1 1
    v10 = cast v6 as Field
    v11 = cast v5 as Field
    v12 = mul v10, Field 2 // NOT(v5) * 2
    v13 = mul v11, v7 // v5 * 0
    v14 = add v12, v13
    store v14 at v2
    v15 = load v2
    v16 = eq v15, Field 2
    constrain v15 == Field 2
    return 
}

This branch

acir(inline) fn main f0 {
  b0(v0: Field):
    v1 = allocate
    v2 = allocate
    store Field 0 at v2
    v5 = eq v0, Field 10
    v6 = not v5
    enable_side_effects v5
    v7 = load v2
    store Field 2 at v2
    v9 = not v5 
    store v7 at v2
    enable_side_effects u1 1
    v11 = cast v5 as Field
    v12 = cast v9 as Field
    v13 = mul v11, Field 2 // v5 * 2
    v14 = mul v12, v7 // NOT(v5) * 0
    v15 = add v13, v14
    store v15 at v2
    v16 = load v2
    v17 = eq v16, Field 2
    constrain v16 == Field 2
    return 
}

@TomAFrench TomAFrench marked this pull request as draft September 4, 2024 12:52
@vezenovm
Copy link
Contributor

vezenovm commented Sep 4, 2024

This branch changes the order of the jmpif as expected in the simplify_cfg pass however the issue comes up in flattening as we end up merging the values incorrectly.

If we do not see an simple or obvious way to have this optimization work w/ flattening we could make this optimization just work on Brillig for now.

@jfecher
Copy link
Contributor

jfecher commented Sep 4, 2024

I'd like if we investigated why flattening isn't adapting to the new changes. Could be related to the bug I've been looking at.

@TomAFrench
Copy link
Member Author

Agreed, we should at least understand the root cause of this first.

TomAFrench and others added 4 commits September 10, 2024 13:42
* master: (60 commits)
  fix: suggest trait attributes in LSP (#5972)
  fix: Error when `quote` is used in runtime code (#5978)
  chore: document HashMap (#5984)
  fix: Restrict keccak256_injective test input to 8 bits (#5977)
  fix: Error when comptime functions are used in runtime code (#5976)
  chore: document BoundedVec (#5974)
  feat: add `Expr::as_let` (#5964)
  chore: remove 3 unused functions warnings in the stdlib (#5973)
  feat: let `nargo` and LSP work well in the stdlib (#5969)
  feat: show doc comments in LSP (#5968)
  feat: add a `panic` method to the stdlib (#5966)
  fix: LSP document symbol didn't work for primitive impls (#5970)
  fix(mem2reg): Handle aliases in function last store cleanup and additional alias unit test (#5967)
  fix: let `derive(Eq)` work for empty structs (#5965)
  feat: add `FunctionDefinition` methods `is_unconstrained` and `set_unconstrained` (#5962)
  feat: LSP autocompletion for attributes (#5963)
  feat: `Module::add_item` (#5947)
  feat: Add `StructDefinition::add_generic` (#5961)
  feat: Add `StructDefinition::name` (#5960)
  fix(mem2reg): Handle aliases better when setting a known value for a load (#5959)
  ...
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.

3 participants