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): Add loop unrolling pass #1364

Merged
merged 33 commits into from
May 24, 2023
Merged

chore(ssa refactor): Add loop unrolling pass #1364

merged 33 commits into from
May 24, 2023

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented May 17, 2023

Related issue(s)

Resolves #(none)

Description

Summary of changes

Adds a loop unrolling pass to the ssa refactoring. This pass currently does expect all loops to have constant indices and be unroll-able, so it will fail if it encounters a non-constant loop. As a result, this pass should currently take place only after function inlining when any comptime parameters used as loop indices become known. This limitation is not inherent to the pass however, we just need to modify find_next_loop to return all loops instead of one (only 1 line needs to change for this), add bookkeeping to remember which loops failed to inline, and refresh the list of loops when unrolling one loop modifies the blocks in another (this happens with nested loops). This limitation is no longer the case - I've updated the PR. Loops using non-constant indices now are left unmodified which allows us some more flexibility in ssa pass ordering.

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

Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

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

Looks good -- just a few comments

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.

I think the test would benefit from being more explicit about the expected result. The current result looks wrong to me.

@joss-aztec joss-aztec added this pull request to the merge queue May 24, 2023
Merged via the queue into master with commit a21b251 May 24, 2023
@joss-aztec joss-aztec deleted the jf/ssa-blocks branch May 24, 2023 10:47
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.

3 participants