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(ssa): Switch mem2reg pass to be per function rather than per block #2243

Merged
merged 31 commits into from
Aug 15, 2023

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Aug 9, 2023

Description

Update mem2reg to be ran per function rather than per block. This enables mem2reg to be easily reordered amongst the SSA pass, such as being placed before loop unrolling. The previous per block mem2reg would break if placed before the flattening of the CFG.

Problem*

Resolves #2218

Summary*

The main change in this PR is that we now have to analyze the CFG when we want to find a load to substitute and fetch the respective load value to be substituted. Three new methods have been made:

fetch_load_value_to_substitute

  • This method fetches the already save load values to substitute for a given address
    The search starts at the block supplied as a parameter. If there is not a load to substitute
    the CFG is analyzed to determine whether a predecessor block has a load value to substitute.
    If there is no load value to substitute the original address is returned.

find_load_to_substitute

  • This method determines which loads should be substituted.
    Starting at the block supplied as a parameter, we check whether a store has occurred with the given address.
    If no store has occurred in the supplied block, the CFG is analyzed to determine whether
    a predecessor block has a store at the given address.

has_common_successor

  • This method checks whether the block we are examining shares a common successor with its predecessor block. This check is necessary to prevent removing stores across blocks whose execution is dependent upon a runtime if statement. Without this check we could potentially remove a store and replace its load for only one block of a jmp if. We should be waiting for flattening to handle both scenarios. Thus, the mem2reg pass should also be ran after flattening to handle any runtime if statements.

We maintain two maps for loads to substitute. Per function, and per block. As when it comes time to actually substitute load result values and delete load instructions we want it to be done in the correct block order. We could most likely come-up with a strategy to use the per function loads to substitute map for replacing load result values, but simply maintaining a separate map felt like a simpler approach.

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

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

@vezenovm vezenovm changed the title cleanup load instruction search w/ predecessors methods feat: Switch mem2reg pass to be per function rather than per block Aug 9, 2023
@vezenovm vezenovm changed the title feat: Switch mem2reg pass to be per function rather than per block feat(ssa): Switch mem2reg pass to be per function rather than per block Aug 9, 2023
@vezenovm vezenovm marked this pull request as ready for review August 10, 2023 15:42
@vezenovm vezenovm requested a review from jfecher August 10, 2023 15:43
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.

I haven't finished a full review yet - I'll have to look at this more in depth tomorrow. I'd like to test it on some examples as well.

crates/noirc_evaluator/src/ssa/opt/mem2reg.rs Outdated Show resolved Hide resolved
crates/noirc_evaluator/src/ssa/opt/mem2reg.rs Outdated Show resolved Hide resolved
crates/noirc_evaluator/src/ssa/opt/mem2reg.rs Outdated Show resolved Hide resolved
crates/noirc_evaluator/src/ssa/opt/mem2reg.rs Outdated Show resolved Hide resolved
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.

I'm still not 100% confidant in this pass honestly since it is getting harder to keep the corner cases in my head as the pass gets more complex but my thoughts are we should merge it (after some minor comments) since it passes all the tests and flag any issues for fixing later. I have to make some changes to mem2reg to support alias analysis now as well so I'll be making some other large changes to the pass in the future.

jfecher
jfecher previously approved these changes Aug 15, 2023
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.

👍

@vezenovm
Copy link
Contributor Author

@jfecher Could you approve once more?

jfecher
jfecher previously approved these changes Aug 15, 2023
@jfecher jfecher enabled auto-merge August 15, 2023 20:15
@jfecher jfecher added this pull request to the merge queue Aug 15, 2023
Merged via the queue into master with commit 0d548b9 Aug 15, 2023
15 checks passed
@jfecher jfecher deleted the mv/func-mem2reg branch August 15, 2023 22:07
TomAFrench added a commit that referenced this pull request Aug 16, 2023
* master: (34 commits)
  chore: move orphaned integration tests to new directory (#2331)
  chore(noir): Release 0.10.1 (#2328)
  feat(ssa): Switch mem2reg pass to be per function rather than per block (#2243)
  feat(ssa): Perform dead instruction elimination on intrinsic functions (#2276)
  feat: Add full call stacks to runtime errors (#2310)
  chore(ci): fix mismatched input name to publish workflow (#2327)
  chore: add README for integration test structure (#2277)
  feat: Improved error message for unexpected return type (#2302)
  feat(stdlib): Implement `str` `as_bytes` and `into_bytes` function (#2298)
  chore(ci): automatically convert changelog entries to sentence case (#2325)
  chore(noir): Release 0.10.0 (#2039)
  fix(lsp): Ensure lsp does not crawl past the root specified (#2322)
  fix: Prevent panic when passing relative paths to `--program-dir` (#2324)
  fix: Overflowing assignment will result in an error (#2321)
  chore: clippy fixes (#2320)
  chore: Parameterize the build mode for noir-wasm (#2317)
  chore: Make `wasm` tests pull from `result` directory (#2319)
  chore: Fix typo (#2315)
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  ...
TomAFrench added a commit that referenced this pull request Aug 17, 2023
* master: (25 commits)
  chore: update noir-source-resolver from `1.1.2` to `^1.1.3` (#2349)
  chore(ci): Avoid writing to cache in workflows triggered by the merge queue (#2341)
  chore(noir): Release 0.10.3 (#2344)
  feat(lsp): Add `Execute` code lens for `main` functions (#2330)
  feat(lsp): Add `Compile` code lens for `main` function and contracts (#2309)
  feat: Allow calling higher-order functions with closures (#2335)
  fix: Display warning if last expression of block is unused (#2314)
  chore(noir): Release 0.10.2 (#2343)
  fix: Prevent dead instruction elimination of brillig functions which may contain side-effects (#2340)
  chore: Separate integration tests for contracts and programs (#2339)
  chore: move orphaned integration tests to new directory (#2331)
  chore(noir): Release 0.10.1 (#2328)
  feat(ssa): Switch mem2reg pass to be per function rather than per block (#2243)
  feat(ssa): Perform dead instruction elimination on intrinsic functions (#2276)
  feat: Add full call stacks to runtime errors (#2310)
  chore(ci): fix mismatched input name to publish workflow (#2327)
  chore: add README for integration test structure (#2277)
  feat: Improved error message for unexpected return type (#2302)
  feat(stdlib): Implement `str` `as_bytes` and `into_bytes` function (#2298)
  chore(ci): automatically convert changelog entries to sentence case (#2325)
  ...
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.

SSA: Enable mem2reg to be per function rather than per block
2 participants