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

[Arc] Add dominance-aware pass to sink ops and merge scf.if ops #7702

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

fabianschuiki
Copy link
Contributor

Add the MergeIfs pass to the Arc dialect. This pass covers a handful of control flow optimizations that are valuable to pick up after hw.modules have been linearized and lowered into arc.model ops:

  • It moves operations closer to their earliest user, if possible sinking them into blocks if all uses are nested in the same block.
  • It merges adjacent scf.if operations with the same condition.
  • It moves operations in between two scf.if operations ahead of the first if op to allow them to be merged.

The MergeIfs pass can operate on SSACFG regions. It assigns an integer order to each operation and considers that order to determine up to which point operations can be moved without moving beyond their first use and without crossing interfering side-effecting ops. The pass is aware of side-effects, and in particular uses the non-aliasing between arc.state and arc.memory to track read/write side-effects at a per-state level, which allows for fairly aggressive optimization. Other side-effecting ops act as a hard barrier and will not be moved or moved over.

This pass supersedes the very effective GroupResetsAndEnables pass we have been using until now. The latter relies on the LegalizeStateUpdate pass to run at a later point however, which will be removed in a future PR, thus making this new pass necessary.

This is a preparatory step for a later PR that overhauls the LowerState pass. That rewrite will make the arc.clock_tree and arc.passthrough ops obsolete, which is why they are not present in the tests.

@fabianschuiki fabianschuiki added the Arc Involving the `arc` dialect label Oct 14, 2024
@fabianschuiki fabianschuiki requested review from TaoBi22 and maerhart and removed request for maerhart October 14, 2024 02:40
Copy link
Contributor

@TaoBi22 TaoBi22 left a comment

Choose a reason for hiding this comment

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

This is super cool! Very cool that this detects and works with non-overlapping side effects too!

lib/Dialect/Arc/Transforms/MergeIfs.cpp Outdated Show resolved Hide resolved
lib/Dialect/Arc/Transforms/MergeIfs.cpp Outdated Show resolved Hide resolved
lib/Dialect/Arc/Transforms/MergeIfs.cpp Outdated Show resolved Hide resolved
Base automatically changed from fschuiki/arc-ssacfg-model to main October 14, 2024 16:57
Copy link
Member

@maerhart maerhart left a comment

Choose a reason for hiding this comment

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

Awesome work! 🎉

lib/Dialect/Arc/Transforms/MergeIfs.cpp Outdated Show resolved Hide resolved
lib/Dialect/Arc/Transforms/MergeIfs.cpp Outdated Show resolved Hide resolved
lib/Dialect/Arc/Transforms/MergeIfs.cpp Outdated Show resolved Hide resolved
lib/Dialect/Arc/Transforms/MergeIfs.cpp Outdated Show resolved Hide resolved
lib/Dialect/Arc/Transforms/MergeIfs.cpp Show resolved Hide resolved
lib/Dialect/Arc/Transforms/MergeIfs.cpp Outdated Show resolved Hide resolved
lib/Dialect/Arc/Transforms/MergeIfs.cpp Outdated Show resolved Hide resolved
lib/Dialect/Arc/Transforms/MergeIfs.cpp Outdated Show resolved Hide resolved
lib/Dialect/Arc/Transforms/MergeIfs.cpp Outdated Show resolved Hide resolved
lib/Dialect/Arc/Transforms/MergeIfs.cpp Show resolved Hide resolved
Add the `MergeIfs` pass to the Arc dialect. This pass covers a handful
of control flow optimizations that are valuable to pick up after
`hw.module`s have been linearized and lowered into `arc.model` ops:

- It moves operations closer to their earliest user, if possible sinking
  them into blocks if all uses are nested in the same block.
- It merges adjacent `scf.if` operations with the same condition.
- It moves operations in between two `scf.if` operations ahead of the
  first if op to allow them to be merged.

The `MergeIfs` pass can operate on SSACFG regions. It assigns an integer
order to each operation and considers that order to determine up to
which point operations can be moved without moving beyond their first
use and without crossing interfering side-effecting ops. The pass is
aware of side-effects, and in particular uses the non-aliasing between
`arc.state` and `arc.memory` to track read/write side-effects at a
per-state level, which allows for fairly aggressive optimization. Other
side-effecting ops act as a hard barrier and will not be moved or moved
over.

This pass supersedes the very effective `GroupResetsAndEnables` pass
we have been using until now. The latter relies on the
`LegalizeStateUpdate` pass to run at a later point however, which will
be removed in a future PR, thus making this new pass necessary.

This is a preparatory step for a later PR that overhauls the LowerState
pass. That rewrite will make the `arc.clock_tree` and `arc.passthrough`
ops obsolete, which is why they are not present in the tests.
@fabianschuiki fabianschuiki merged commit 2085d0d into main Oct 15, 2024
4 checks passed
@fabianschuiki fabianschuiki deleted the fschuiki/arc-merge-ifs branch October 15, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arc Involving the `arc` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants