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): Expand RC optimization pass to search return block for inc_rcs #6116

Closed
wants to merge 6 commits into from

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Sep 20, 2024

Description

Problem*

Resolves #6088 (comment) (at least attempts to do so)

Part of general effort to lower Brillig bytecode sizes.

Summary*

inc/dec rc instructions take 3 brillig opcodes. We should look to remove unnecessary ones. The current RC opt pass is very conservative. This PR is an initial effort to expand the scope of the RC opt pass.

We currently only search for inc_rc instructions issued in the entry block. This greatly simplifies the pass as we do not have to account for inc/dec rc in loops. However, a simple low-hanging fruit would be to also search for inc_rc instructions in an exit block with a return terminator as we know that we are no longer in a loop after the exit block is executed.

I also updated the array sets scan to not just keep all inc rcs of the same type. Now we keep all rcs with an array set of the same type where rc instruction has the same value or the rc value comes from a function of block param.

I also moved the RC opt pass to happen after simplifying the cfg. After simplification we may have blocks that can be simplified into the return block for which we would not be catching these possible optimizations.

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 20, 2024

Changes to Brillig bytecode sizes

Generated at commit: 7a909da36582d7f71c9e0e23630476fa9c414cd4, compared to commit: 5b27ea4d8031318723cc2b97f76758d401a565a0

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
poseidon2 -6 ✅ -2.08%
conditional_2 -6 ✅ -5.71%

Full diff report 👇
Program Brillig opcodes (+/-) %
fold_numeric_generic_poseidon 701 (-6) -0.85%
no_predicates_numeric_generic_poseidon 701 (-6) -0.85%
poseidon2 282 (-6) -2.08%
conditional_2 99 (-6) -5.71%

@vezenovm vezenovm marked this pull request as ready for review September 20, 2024 18:07
@vezenovm
Copy link
Contributor Author

vezenovm commented Sep 20, 2024

Hmm it does feel like marginal benefits for running this pass again. Although in general we can expand the scope of the RC opt pass, perhaps just not with this PR. I'll leave it up for comments though.

This mainly removes paired inc/dec rc instructions on arrays returned from black boxes.
e.g we see this pattern often:

    v62 = call poseidon2_permutation(v61, u32 4)
    inc_rc v62
    dec_rc v62

Where v62 is never used in an array set. Although the conditional_2 test also showed an improvement and does not call to a black box.

@vezenovm vezenovm requested review from a team and jfecher September 20, 2024 18:12
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

Benefits seem a bit marginal to me to add another SSA pass for it.
It'd be nice if we had timing metrics for the compiler itself so we could see exactly how much new compiler passes impact it.

@vezenovm
Copy link
Contributor Author

Benefits seem a bit marginal to me to add another SSA pass for it.

Yeah I agree. I'm going to look at how I can continue to make the pass less conservative where it may justify running the pass again.

It'd be nice if we had timing metrics for the compiler itself so we could see exactly how much new compiler passes impact it.

Agreed, that would definitely helpful info as we continue to update the SSA pass.

@TomAFrench TomAFrench added the run-external-checks Trigger CI job to run tests on external repos label Sep 25, 2024
@vezenovm
Copy link
Contributor Author

Closing as we solved the poseidon case this PR was looking to solve by searching for these useless inc_rc/dec_rc pairing per block rather than per function #6160. #6160 also applied optimizations to more tests than this PR.

@vezenovm vezenovm closed this Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-external-checks Trigger CI job to run tests on external repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants