[LowerTypes] Preserve rwprobes, forceable. #5051

dtzSiFive · 2023-04-14T20:56:43Z

Don't lower these as much as possible.

Keep the always-lower-(non-rw-)probe behavior
to workaround the memtaps / 4479 issue.

Also, if we need to lower the source operand,
re-construct/materialize it (like "Source Materialization").

This does of course mean rwprobe'ing something means it needs to be okay to preserve as an aggregate.

dtzSiFive · 2023-04-14T21:08:32Z

~~This might be best to land after some support for ref.sub in LowerXMR, or without the ability to split up force statements (which isn't ideal anyway) even when the source operand is being expanded.~~

Fixed, source operand is re-built if needed for a force (but for whatever reason expanded elsewhere).
This is a little weird, but is basically "Source Materialization" re:retaining uses of the "illegal" type.

No longer split up force or release or forceable (with rwprobe result active), ever, that code is removed.

This also changes things so rwprobe's are never lowered, regardless of convention, which seems reasonable if you squint they could count as "scalars".

dtzSiFive · 2023-04-15T00:19:19Z

Wargarbl, connections to forceable declarations get sad with this unless agg preservation is on:

https://github.com/dtzSiFive/reftype-inputs/blob/main/force_agg.fir

Gives error:

./ref_tests/force_agg.fir:26:7: error: 'hw.struct_extract' op used as connect destination
    p <= x
      ^
./ref_tests/force_agg.fir:26:7: note: see current operation: %7 = "hw.struct_extract"(%5) {field = "a"} : (!hw.struct<a: i2, b: !hw.array<3xi2>>) -> i2
./ref_tests/force_agg.fir:26:7: error: 'firrtl.strictconnect' op LowerToHW couldn't handle this operation
    p <= x
      ^
./ref_tests/force_agg.fir:26:7: note: see current operation: "firrtl.strictconnect"(%8, %1) : (!firrtl.uint<2>, !firrtl.uint<2>) -> ()

Bit of a balancing act, will think/bake this some more...

uenoku · 2023-04-16T12:49:33Z

I think this error was because we started to reject subelement connection from HW wire PR: #4826

To fix this I guess it's sufficient to enable MergeConnection pass even when with non-aggregate preservation (currently ran only when preserveAggregate != None).

fabianschuiki · 2023-04-20T21:45:42Z

Yeah there was a version of lowerConnect in the hw.wire PRs that would allow you to connect to subaccesses on the LHS, and it would basically refactor the connect to be to the whole non-subaccessed value, with an updated value on the RHS. But MergeConnection does that in a more orthogonal way, which is why it's missing on the lowerConnect side.

dtzSiFive · 2023-04-25T23:24:58Z

Thank you both very much! I'll look into that, enabling MergeConnections does resolve that error! And appreciate the context/analysis.

dtzSiFive · 2023-04-26T16:05:04Z

Note: If MergeConnections is required for correctness (legalization), it shouldn't be gated on "disable-opts". This is not yet a concern, but should be fixed as part of landing this PR.

EDIT: Discussed in #5086, fixed in 9ed6ecf .

lib/Dialect/FIRRTL/Transforms/LowerTypes.cpp

uenoku

Few nits, otherwise LGTM!

uenoku · 2023-04-27T02:11:54Z

lib/Dialect/FIRRTL/Transforms/LowerTypes.cpp

+        .Case<RefForceOp, RefForceInitialOp, RefSendOp, VectorCreateOp,
+              BundleCreateOp>([&](auto op) {
+          // Reconstruct the aggregate for the ref operation.
+          // (or vector/bundle create that were inserted.. woof.)
+          ImplicitLocOpBuilder b(user->getLoc(), user);
+
+          Value input =
+              TypeSwitch<Type, Value>(val.getType())
+                  .template Case<FVectorType>([&](auto vecType) {
+                    return b.createOrFold<VectorCreateOp>(vecType, mapping);
+                  })
+                  .template Case<BundleType>([&](auto bundleType) {
+                    return b.createOrFold<BundleCreateOp>(bundleType, mapping);
+                  })
+                  .Default([&](auto _) -> Value { return {}; });
+          if (!input) {
+            user->emitError("unable to reconstruct source of type ")
+                << val.getType();
+            return;
+          }


I think we can use for Default as well. Default currently creates aggrgates by bitcast operations (since there was no vector/bundle_create at that time) so it would be way better to explicitly use vector/bundle_create.

Example: firtool -preserve-aggregate=1d-vec.

circuit Bar: module Bar: input a1: UInt<1>[2] input a2: UInt<1>[2] input cond: UInt<1> output b: UInt<1>[2] b <= mux(cond, a1, a2)

As an aside: I've noticed playing with aggregates that often names become _GEN -- if we're exploding then reconstructing that would sure explain that. Unfortunately this doesn't (easily) lend itself to addressing that, but it'd be neat if so.

In your example you're showing another case where we "unevenly" lower things--here, the scalarization of the top module causes the vectors to be split and we'd prefer a vector/bundle create to recover those pieces vs bitcast.
Is that right?
(Makes lots of sense to me!)

Interesting point!! I am unsure... okay digging a little it appears this code was written before we had aggregate creation ops 😄 .
(( Depending on how you look at the purpose of LowerTypes, I was concerned the mechanism for materializing the aggregate might be important -- for claims like "No FIRRTL op will still take an aggregate" or if the story for foreign ops made more sense giving it an opaque blob of bits directly. In that sense this was a little suboptimal but I didn't have a better story other than rejecting. ))

Anyway a few conversations later I think this makes sense. I'll change it separately.

cc #3989 which might be useful to revive here re:neater IR but not 100% as haven't gone through that carefully.

Don't lower these as much as possible. Keep the always-lower-(non-rw-)probe behavior to workaround the memtaps / 4479 issue. Also, if we need to lower the source operand to a force statement, split it up as well. Some sharp corners here re:references to non-passive things, can be handled similarly to the force situation-- just reconstruct or cat+cast the elements back.

If not splitting the force, reconstruct the source operands from the expanded elements. This is similar to source materialization in MLIR's conversion.

Leave the boolean simplify thing along for note re:memtaps issue.

Undo change that used the input argument, which was motivated by ensuring a ref.send of a non-passive would be lowered since we always lower non-passive types. Instead, always look at the ref result for determining this, so that ref chains are consistent. We can rebuild the aggregate into the send as needed. As-is we always expand read-only references anyway, so this just ensures the send is also split apart using same logic splitting up ref.define/etc of probe's.

Per reviewer feedback, thanks!

dtzSiFive · 2023-04-27T19:28:56Z

Thank you all very much for the feedback and review! Will make the suggested change re:improving other paths in an immediate follow-up.

(also makes me happy to not make this so specific to references... 👍)

dtzSiFive requested a review from darthscsi April 14, 2023 20:56

dtzSiFive force-pushed the experimental/dont-force-lower-reftypes branch from e8d739a to c7f2cca Compare April 14, 2023 20:59

dtzSiFive requested review from prithayan and uenoku April 14, 2023 23:42

dtzSiFive mentioned this pull request Apr 26, 2023

[FIRRTL] firtool: Run merge-cnxns regardless of agg preserv. #5086

Merged

dtzSiFive force-pushed the experimental/dont-force-lower-reftypes branch 2 times, most recently from 69ad6b4 to b621efd Compare April 26, 2023 02:02

uenoku reviewed Apr 27, 2023

View reviewed changes

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

uenoku approved these changes Apr 27, 2023

View reviewed changes

dtzSiFive added 5 commits April 27, 2023 14:02

[LowerTypes] reconstruct vector/bundles for forces/send ops.

76c30d2

If not splitting the force, reconstruct the source operands from the expanded elements. This is similar to source materialization in MLIR's conversion.

[LowerTypes] appease tidy.

4ee71af

Leave the boolean simplify thing along for note re:memtaps issue.

[FIRRTL][LowerTypes] Rename "type" arg, add comment about lowerProducer.

12affab

Per reviewer feedback, thanks!

dtzSiFive force-pushed the experimental/dont-force-lower-reftypes branch from b621efd to 12affab Compare April 27, 2023 19:04

dtzSiFive merged commit cacff52 into llvm:main Apr 27, 2023

dtzSiFive deleted the experimental/dont-force-lower-reftypes branch April 27, 2023 19:32

dtzSiFive mentioned this pull request Oct 6, 2023

[FIRRTL] Disallow rwprobe/forceable of non-passive. #6263

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LowerTypes] Preserve rwprobes, forceable. #5051

[LowerTypes] Preserve rwprobes, forceable. #5051

dtzSiFive commented Apr 14, 2023 •

edited

Loading

dtzSiFive commented Apr 14, 2023 •

edited

Loading

dtzSiFive commented Apr 15, 2023

uenoku commented Apr 16, 2023

fabianschuiki commented Apr 20, 2023

dtzSiFive commented Apr 25, 2023

dtzSiFive commented Apr 26, 2023 •

edited

Loading

uenoku left a comment

uenoku Apr 27, 2023 •

edited

Loading

dtzSiFive Apr 27, 2023

dtzSiFive commented Apr 27, 2023

[LowerTypes] Preserve rwprobes, forceable. #5051

[LowerTypes] Preserve rwprobes, forceable. #5051

Conversation

dtzSiFive commented Apr 14, 2023 • edited Loading

dtzSiFive commented Apr 14, 2023 • edited Loading

dtzSiFive commented Apr 15, 2023

uenoku commented Apr 16, 2023

fabianschuiki commented Apr 20, 2023

dtzSiFive commented Apr 25, 2023

dtzSiFive commented Apr 26, 2023 • edited Loading

uenoku left a comment

Choose a reason for hiding this comment

uenoku Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

dtzSiFive Apr 27, 2023

Choose a reason for hiding this comment

dtzSiFive commented Apr 27, 2023

dtzSiFive commented Apr 14, 2023 •

edited

Loading

dtzSiFive commented Apr 14, 2023 •

edited

Loading

dtzSiFive commented Apr 26, 2023 •

edited

Loading

uenoku Apr 27, 2023 •

edited

Loading