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

[MapArith] Introduce --map-arith-to-comb pass #5297

Merged
merged 5 commits into from
Jun 6, 2023

Conversation

mortbopet
Copy link
Contributor

Adds a pass which does a simple arith to comb mapping. The intention of this pass is mainly to get code out of Handshake (and wherever else we do some ad-hoc, simple arith-to-comb mapping/HLS), thus allowing integration testing of DC flows (refresher: handshake-to-dc lowers handshake ops to a combination of dc and arith).

This does not intend to be the definitive lowering/HLS pass of arith operations (hence the name "map" instead of e.g. "lower"). See it more as a temporary pass to provide this kind of functionality until we have an operator library + library mapping pass.

Currently, this pass strictly does operation mapping. If needed, index-type conversion could also be moved to this pass (e.g. setting index type width as a parameter of the pass).

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.

Just some drive-by suggestions.

test/Transforms/map-arith-to-comb.mlir Show resolved Hide resolved
lib/Transforms/MapArithToComb.cpp Outdated Show resolved Hide resolved
lib/Transforms/MapArithToComb.cpp Outdated Show resolved Hide resolved
LogicalResult
matchAndRewrite(arith::ExtSIOp op, OpAdaptor adaptor,
ConversionPatternRewriter &rewriter) const override {
size_t outWidth = op.getType().getIntOrFloatBitWidth();
Copy link
Member

Choose a reason for hiding this comment

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

arith also supports vectors and tensors in which case this would just crash. Maybe add a TypeConverter which only marks IntegerType legal or check it otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch - we definitely want to make it clear what things can and can't be trivially converted with this pass.

Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

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

Having this pass makes sense to me, we could use it in several places in lieu of a proper low-level operator library.

include/circt/Transforms/Passes.td Outdated Show resolved Hide resolved
Adds a pass which does a simple `arith` to `comb` mapping. The intention of this pass is mainly to get code out of Handshake (and wherever else we do some ad-hoc, simple arith-to-comb mapping/HLS), thus allowing integration testing of DC flows (refresher: handshake-to-dc lowers handshake ops to a combination of `dc` and `arith`).

This **does not** intend to be the definitive lowering/HLS pass of `arith` operations (hence the name "map" instead of e.g. "lower"). See it more as a temporary pass to provide this kind of functionality until we have an operator library + library mapping pass.

Currently, this pass strictly does operation mapping. If needed, `index`-type conversion could also be moved to this pass (e.g. setting `index` type width as a parameter of the pass).
mortbopet added a commit that referenced this pull request Jun 6, 2023
This commit adds a lowering of DC operation to a combination of
`comb,seq` and `hw` operations. The implementations are identical to
those of the `handshake` dialect (i.e. `handshake-to-hw` sans the
arithmetic operations).

- ESI-typed I/O
- DC buffers lower to ESI buffers
  - This is just a temporary solution. I anticipate more complexity here
    in the future when we want to do more with the FIFOs that connect
    DC circuits.
- In case the IR contains DC operations that need to be clocked (fork, buffer),
  there must exist a clock and reset signal in the parent `FunctionLike`
  operation. These arguments are to be marked with a `dc.clock` and `dc.reset`
  attribute, respectively.
- Only a single integration test for now. Will add more once #5297 lands
  (allows us to do some high-level tests through handshake).
@mortbopet mortbopet merged commit 5a1f999 into main Jun 6, 2023
5 checks passed
mortbopet added a commit that referenced this pull request Jun 20, 2023
This commit adds a lowering of DC operation to a combination of
`comb,seq` and `hw` operations. The implementations are identical to
those of the `handshake` dialect (i.e. `handshake-to-hw` sans the
arithmetic operations).

- ESI-typed I/O
- DC buffers lower to ESI buffers
  - This is just a temporary solution. I anticipate more complexity here
    in the future when we want to do more with the FIFOs that connect
    DC circuits.
- In case the IR contains DC operations that need to be clocked (fork, buffer),
  there must exist a clock and reset signal in the parent `FunctionLike`
  operation. These arguments are to be marked with a `dc.clock` and `dc.reset`
  attribute, respectively.
- Only a single integration test for now. Will add more once #5297 lands
  (allows us to do some high-level tests through handshake).
mortbopet added a commit that referenced this pull request Jun 20, 2023
This commit adds a lowering of DC operation to a combination of
`comb,seq` and `hw` operations. The implementations are identical to
those of the `handshake` dialect (i.e. `handshake-to-hw` sans the
arithmetic operations).

- ESI-typed I/O
- DC buffers lower to ESI buffers
  - This is just a temporary solution. I anticipate more complexity here
    in the future when we want to do more with the FIFOs that connect
    DC circuits.
- In case the IR contains DC operations that need to be clocked (fork, buffer),
  there must exist a clock and reset signal in the parent `FunctionLike`
  operation. These arguments are to be marked with a `dc.clock` and `dc.reset`
  attribute, respectively.
- Only a single integration test for now. Will add more once #5297 lands
  (allows us to do some high-level tests through handshake).
@darthscsi darthscsi deleted the dev/mpetersen/map_arith branch June 4, 2024 14:48
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.

None yet

3 participants