[FIRRTL] Add start of FIRRTL types lowering #157

mikeurbach · 2020-10-16T04:19:57Z

This adds a new pass to lower FIRRTL types, initially focused on
bundle types. This flattens bundle types in module and instance
ports, and deals with the flattened values in subfield and connect
operations. Starts to resolve #123.

include/circt/Dialect/FIRRTL/Dialect.h

include/circt/Dialect/FIRRTL/Types.h

lib/Dialect/FIRRTL/Ops.cpp

test/e2e/simple_addi.mlir

test/firrtl/lower-types.mlir

lib/Dialect/FIRRTL/PassDetails.h

mikeurbach · 2020-10-16T04:41:43Z

@seldridge @lattner here is the start of lowering FIRRTL types (cc @fabianschuiki @teqdruid who were also interested). I'm pretty happy with the functionality (i.e. the test cases I was able to write), but I'm sure I have more to clean up. If you have any feedback at this point, that would be great.

I've really only focused on flattening bundle types here. It's already a pretty hefty chunk of code, so I'd like to try and get this solidified, and then we can work on fleshing out more of this pass incrementally.

lattner

I didn't review the logic of the pass closely, but the code looks nice and clean in a quick glance. Here are some quick comments.

include/circt/Dialect/FIRRTL/Dialect.h

include/circt/Dialect/FIRRTL/Types.h

lib/Dialect/FIRRTL/Ops.cpp

lib/Dialect/FIRRTL/LowerTypes.cpp

fabianschuiki

That's super nice! Will make the FIRRTL-to-LLHD lowering so much simpler as well.

.github/workflows/buildAndTest.yml

mikeurbach · 2020-10-21T19:39:21Z

@lattner @seldridge I've done a round of updates from the previous feedback, and dressed up the tests. I think this should be ready for review now. Thanks!

mikeurbach

I was going to add some tests for the ways this pass can error out, but now I'm not sure it is even necessary to emit an error (instead of asserting). The places I have emitError are only possible if the fields of bundles don't line up, or the subfields weren't traversed correctly, etc. To me, it seems like these sorts of checks belong in the verifier, but I could go either way.

lib/Dialect/FIRRTL/LowerTypes.cpp

mikeurbach · 2020-11-04T19:01:03Z

Sorry @fabianschuiki meant to request your review on #200.

fabianschuiki

LGTM from the LLHD side 😃 👍

lattner

This is looking really nice, lots of minor suggestions and thoughts, but the code is very clean!

lib/Dialect/FIRRTL/PassDetails.h

tools/firtool/firtool.cpp

lib/Dialect/FIRRTL/Types.cpp

lib/Dialect/FIRRTL/LowerTypes.cpp

mikeurbach · 2020-11-10T19:36:26Z

@lattner your review was requested automatically, but FYI I still have work in progress. So far, I rebased master and removed the unrelated changes in the previous revision. I also went through and updated the places you had minor feedback. Now, I'm going to try and think about how to improve the data structures in order to generally simplify the pass.

mikeurbach · 2020-11-12T01:41:09Z

@lattner please take another look when you get a chance. I've updated the data structures along the lines of what you were suggesting, and I've been able to simplify this pass a lot. Thanks for the push in the right direction!

lattner

This looks really awesome @mikeurbach great work. A few more comments below, but feel free to commit after you resolve them however you think best. No need for another round of review from me :)

lib/Dialect/FIRRTL/LowerTypes.cpp

lattner · 2020-11-13T05:57:00Z

lib/Dialect/FIRRTL/LowerTypes.cpp

+  // Flatten any bundle types.
+  SmallVector<FlatBundleFieldEntry, 8> fieldTypes;
+  flattenBundleTypes(type, "", false, fieldTypes);
+


Would it make sense to add a check to see if we are lowering an argument to itself here to avoid needlessly rewriting the IR?

Something like:

if (fieldTypes.size() == 1 && (other checks?)) return;

Good point, I will try to add early exit for those cases.

Actually, the reason I wasn't exiting early for the unchanged arguments was to keep the resulting arguments in the same relative order (with bundles flattened of course). I achieve this by copying unchanged arguments to the end of the argument list and rewriting the IR to use the copy.

I think the proper thing to do here is twofold: first, any time we lower a bundle argument, we should just insert the lowered values right after the original bundle in the argument list. Then, we don't need to needlessly copy/rewrite for every unchanged argument to keep the same order.

I will try to take this approach here.

And now I remember why this was tricky--as we lower arguments, if we insert a flattened argument into the middle of the argument list, this may interfere with any argument attributes for the original argument that was at that index. I think it may be possible, but the bookkeeping gets more complicated.

So I see three options:

copy the unchanged arguments and rewrite the IR accordingly, so we can keep the same relative ordering (what is in the current revision)

don't touch unchanged arguments and allow the arguments to change order in the result

insert new arguments into the middle of the argument list and figure out the bookkeeping to keep track of argument attributes

I think 2) is not what we want. It would violate the principle of least surprise, at least for me. For example, output ports would often be arguments at the end of the list, and could get shifted to the middle if some inputs were lowered but outputs were not.

It seems like 3) is the best option in the long run since it would avoid some unnecessary work.

However, I think 1) is sufficient for now. It's doing some unnecessary work in some cases, but the implementation is straightforward and the results are not surprising. I think I'll land this as-is, and if it is worth pursing 3) (or some other approach), I can address it in a follow-up.

I landed this pass and opened this issue to track this specific concern for follow up: #247.

lib/Dialect/FIRRTL/LowerTypes.cpp

This adds a new pass to lower FIRRTL types, initially focused on bundle types. This flattens bundle types in module and instance ports, and deals with the flattened values in subfield and connect operations. Starts to resolve #123.

* New subfields are created immediately after the new instance * They are stored in the central loweredBundleValues map * Subfields of instances are mapped directly to the new subfields

Now, everything is stored in one mapping. There is no difference between how to handle subfields of block args, other subfields, and instances.

Before, it used a DenseMap of Value to StringMap of Value, which was not ideal for having many small string maps. Now, it uses a DenseMap of pairs of (Value, Identifier) to map to Values.

This pipeline can now lower FIRRTL with bundle types to RTL.

lattner · 2020-11-17T01:21:20Z

Great work @mikeurbach !

mikeurbach · 2020-11-17T02:38:22Z

My pleasure. Thanks for all the reviews and suggestions, I really appreciate it!

mikeurbach commented Oct 16, 2020

View reviewed changes

lib/Dialect/FIRRTL/PassDetails.h Show resolved Hide resolved

mikeurbach marked this pull request as draft October 16, 2020 04:36

mikeurbach requested review from seldridge and lattner October 16, 2020 04:36

lattner reviewed Oct 16, 2020

View reviewed changes

fabianschuiki reviewed Oct 16, 2020

View reviewed changes

mikeurbach force-pushed the firrtl-lower-types branch from 8e758aa to aeeb457 Compare October 20, 2020 04:15

mikeurbach commented Oct 21, 2020

View reviewed changes

.github/workflows/buildAndTest.yml Outdated Show resolved Hide resolved

mikeurbach marked this pull request as ready for review October 21, 2020 01:36

mikeurbach force-pushed the firrtl-lower-types branch from bbbaaf6 to dee8a84 Compare October 21, 2020 19:37

github-actions bot requested review from lattner and teqdruid October 21, 2020 19:38

mikeurbach commented Oct 21, 2020

View reviewed changes

lib/Dialect/FIRRTL/LowerTypes.cpp Outdated Show resolved Hide resolved

lib/Dialect/FIRRTL/LowerTypes.cpp Outdated Show resolved Hide resolved

mikeurbach force-pushed the firrtl-lower-types branch from aeb1396 to 03aac9d Compare October 23, 2020 22:30

mikeurbach mentioned this pull request Nov 4, 2020

Update LLVM to 2e36e0dad52 #200

Merged

mikeurbach force-pushed the firrtl-lower-types branch 2 times, most recently from 54634ed to 79302d3 Compare November 4, 2020 15:11

github-actions bot requested a review from fabianschuiki November 4, 2020 18:55

fabianschuiki approved these changes Nov 4, 2020

View reviewed changes

mikeurbach mentioned this pull request Nov 5, 2020

[Handshake] Update MuxOp lowering to use primitive FIRRTL ops #192

Merged

mikeurbach force-pushed the firrtl-lower-types branch from 4ecc367 to fe22dd2 Compare November 5, 2020 02:58

github-actions bot requested a review from fabianschuiki November 5, 2020 02:58

mikeurbach mentioned this pull request Nov 6, 2020

[FIRRTL] LowerToRTL Unhandled op failure #206

Closed

lattner reviewed Nov 7, 2020

View reviewed changes

mikeurbach force-pushed the firrtl-lower-types branch from fe22dd2 to d356a7f Compare November 10, 2020 19:28

github-actions bot requested a review from lattner November 10, 2020 19:29

mikeurbach mentioned this pull request Nov 10, 2020

[llhd-sim] Move trace generation to a dedicated class #229

Merged

mikeurbach force-pushed the firrtl-lower-types branch from 8e4dd88 to ec3bb4c Compare November 12, 2020 01:15

lattner approved these changes Nov 13, 2020

View reviewed changes

mikeurbach added 8 commits November 16, 2020 09:33

[FIRRTL] Add start of FIRRTL types lowering

0fc4ae9

This adds a new pass to lower FIRRTL types, initially focused on bundle types. This flattens bundle types in module and instance ports, and deals with the flattened values in subfield and connect operations. Starts to resolve #123.

Revise instance lowering to use the central storage.

d97b972

* New subfields are created immediately after the new instance * They are stored in the central loweredBundleValues map * Subfields of instances are mapped directly to the new subfields

Revise subfield lowering to use the central storage.

693dae0

Now, everything is stored in one mapping. There is no difference between how to handle subfields of block args, other subfields, and instances.

getBundleLowering cannot fail now, so assert this to simplify things.

af8eca0

Update the central storage data structure.

37f1dd5

Before, it used a DenseMap of Value to StringMap of Value, which was not ideal for having many small string maps. Now, it uses a DenseMap of pairs of (Value, Identifier) to map to Values.

Update some helper names for consistency and clarity, NFC.

d3f8378

Add LowerFIRRTLTypes pass to firtool -lower-to-rtl pipeline.

6756e47

This pipeline can now lower FIRRTL with bundle types to RTL.

Final improvements after last round of review.

5f1cf57

mikeurbach force-pushed the firrtl-lower-types branch from ec3bb4c to 5f1cf57 Compare November 16, 2020 17:34

github-actions bot requested a review from lattner November 16, 2020 17:34

mikeurbach merged commit a0f04b1 into master Nov 16, 2020

mikeurbach deleted the firrtl-lower-types branch November 16, 2020 17:47

mikeurbach mentioned this pull request Nov 16, 2020

[FIRRTL] LowerTypes pass does unnecessary work for arguments that are unchanged #247

Closed

mikeurbach mentioned this pull request Nov 20, 2020

Firrtl instance flips #263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIRRTL] Add start of FIRRTL types lowering #157

[FIRRTL] Add start of FIRRTL types lowering #157

mikeurbach commented Oct 16, 2020

mikeurbach commented Oct 16, 2020

lattner left a comment

fabianschuiki left a comment

mikeurbach commented Oct 21, 2020

mikeurbach left a comment

mikeurbach commented Nov 4, 2020

fabianschuiki left a comment

lattner left a comment

mikeurbach commented Nov 10, 2020

mikeurbach commented Nov 12, 2020

lattner left a comment

lattner Nov 13, 2020

mikeurbach Nov 13, 2020

mikeurbach Nov 16, 2020 •

edited

mikeurbach Nov 16, 2020

mikeurbach Nov 16, 2020 •

edited

lattner commented Nov 17, 2020

mikeurbach commented Nov 17, 2020

[FIRRTL] Add start of FIRRTL types lowering #157

[FIRRTL] Add start of FIRRTL types lowering #157

Conversation

mikeurbach commented Oct 16, 2020

mikeurbach commented Oct 16, 2020

lattner left a comment

Choose a reason for hiding this comment

fabianschuiki left a comment

Choose a reason for hiding this comment

mikeurbach commented Oct 21, 2020

mikeurbach left a comment

Choose a reason for hiding this comment

mikeurbach commented Nov 4, 2020

fabianschuiki left a comment

Choose a reason for hiding this comment

lattner left a comment

Choose a reason for hiding this comment

mikeurbach commented Nov 10, 2020

mikeurbach commented Nov 12, 2020

lattner left a comment

Choose a reason for hiding this comment

lattner Nov 13, 2020

Choose a reason for hiding this comment

mikeurbach Nov 13, 2020

Choose a reason for hiding this comment

mikeurbach Nov 16, 2020 • edited

Choose a reason for hiding this comment

mikeurbach Nov 16, 2020

Choose a reason for hiding this comment

mikeurbach Nov 16, 2020 • edited

Choose a reason for hiding this comment

lattner commented Nov 17, 2020

mikeurbach commented Nov 17, 2020

mikeurbach Nov 16, 2020 •

edited

mikeurbach Nov 16, 2020 •

edited