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

chore(ssa refactor): Update mem2reg pass to work with multiple functions and blocks #1375

Merged
merged 4 commits into from
May 22, 2023

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented May 18, 2023

Related issue(s)

Resolves #

Description

Summary of changes

Previously the mem2reg pass relied on only operating on a single-block program so that it could remove stores properly. I've edited it so that it only removes stores after checking if the program is only one block. With this and some small changes to the entry it should now work on any program. I've also added a test with 2 blocks to test this.

Dependency additions / changes

Test additions / changes

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Documentation needs

  • This PR requires documentation updates when merged.

Additional context

@jfecher jfecher requested review from kevaundray and joss-aztec May 18, 2023 20:08
Copy link
Contributor

@joss-aztec joss-aztec left a comment

Choose a reason for hiding this comment

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

One minor comment on the naming of eliminate_store_load, otherwise LGTM.

I'm curious if you lifted the dfg reference out of the context to address anything in particular or if it was a stylistic change?

crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs Outdated Show resolved Hide resolved
@joss-aztec joss-aztec enabled auto-merge May 22, 2023 17:16
@joss-aztec joss-aztec added this pull request to the merge queue May 22, 2023
Merged via the queue into master with commit a57c574 May 22, 2023
@joss-aztec joss-aztec deleted the jf/mem2reg branch May 22, 2023 18:45
TomAFrench added a commit that referenced this pull request May 24, 2023
* master:
  chore(ssa refactor): Add loop unrolling pass (#1364)
  chore: Add terms added in ssa refactor to `cspell` (#1385)
  chore(ssa refactor): Optimize constant `jmpif`s into `jmp`s (#1374)
  feat: Allow warnings by default (#1383)
  chore(github): Update GitHub Pull Request Template (#1370)
  chore(github): Update GitHub Bug Report Template (#1368)
  chore(ssa refactor): Update mem2reg pass to work with multiple functions and blocks (#1375)
  chore(ssa refactor): Add pass to simplify the control flow graph (#1373)
  chore(github): Update GitHub Feature Request Template (#1369)
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