[FIRRTL] Add RefDefineOp, disallow connecting references otherwise. #4812

dtzSiFive · 2023-03-13T16:08:24Z

Add "define" statement from References Proposal, move all reference connections to use this.

Fixes #4334 .
Fixes #3715 .
Fixes #3713 .

dtzSiFive · 2023-03-13T16:13:53Z

re:RefConnect / Strict-only variant:
The plan was previously to allow non-sized operands to allow inferring through references, starting with "strict"-like (same source/dest type) since that's what we currently require anyway for reference "connections".
(see #4800 for predicate used to define what references may be connected).
This may instead be handled by operation in #4804, perhaps inserting as needed when generating these in the parser + code (emitConnect?).

darthscsi · 2023-03-13T22:39:53Z

lib/Dialect/FIRRTL/FIRRTLOps.cpp

+  // For now, refs can't be in bundles so this is sufficient.
+  // In the future need to ensure no other define's to same "fieldSource".
+  // (When aggregates can have references, we can define a reference within,
+  // but this must be unique.  Checking this here may be expensive,


I've wondered if a number of the more expensive "must be true" but "should be made true and then never changed" things should move to an optional verif pass. Similar to "connect doesn't exist anymore" and maybe "only strict registers exist now". Essentially phase-dependent verification.

We've discussed doing this via a different dialect, or moving a bunch of stuff to CHIRRTL, so on-- but maybe we just lean into "phases" and throw an attribute on the circuit indicating which level/phase it's claiming to be at? Easy to verify when consuming/printing, and between pipeline "phases" (optional, as you say), and we don't need to duplicate things across a bunch of dialects (or sort out what the ideal mid-level dialect should be, yet)?

lib/Dialect/FIRRTL/FIRRTLUtils.cpp

lib/Dialect/FIRRTL/Transforms/ExpandWhens.cpp

lib/Dialect/FIRRTL/Transforms/LowerTypes.cpp

darthscsi · 2023-03-13T22:56:52Z

This may instead be handled by operation in #4804, perhaps inserting as needed when generating these in the parser + code (emitConnect?).

My goal is that strictconnect guarantees identical types and doesn't hide special types (analog, ref, external). I thought I would need to legalize connects, but likely everything can be handled in emitConnect (parsing time). But, that isn't making a statement on how special types should behave w.r.t. inference.

prithayan

Overall this looks good.
Bikeshed: ref.connect instead of ref.define ?

lib/Dialect/FIRRTL/FIRRTLOps.cpp

test/Dialect/FIRRTL/SFCTests/data-taps-xmr.fir

prithayan · 2023-03-13T23:27:35Z

test/Dialect/FIRRTL/ref.mlir

+  firrtl.module private @OutChild(in %x: !firrtl.uint, out %y: !firrtl.uint, out %p: !firrtl.ref<uint>) {
+    %0 = firrtl.ref.send %x : !firrtl.uint
+    firrtl.ref.define %p, %0 : !firrtl.ref<uint>
+    %1 = firrtl.ref.resolve %p : !firrtl.ref<uint>


Maybe we need to make sure LowerXMR can handle this.

Good call. It makes it through firtool, but directly running LowerXMR fails / crashes in a cast looks like, missed that. The issue fixed is specifically about this being rejected by verifier as invalid entirely, which is what this test checks.

Or is there a change/action you're suggesting here?

Something for the TODO re:LowerXMR crashing on this? LowerXMR will need reworking for handling ref.sub and u-turns (#4692) as well, (and aggregates-with-refs eventually), so think this goes on that list.

lib/Dialect/FIRRTL/FIRRTLOps.cpp

uenoku · 2023-03-15T11:07:54Z

lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp

+        auto getRefDefine = [](Value result) -> RefDefineOp {
+          for (auto *user : result.getUsers()) {
+            if (auto rd = dyn_cast<RefDefineOp>(user);
+                rd && rd.getDest() == result)
+              return rd;
+          }
+          return {};
+        };
+        auto rd = getRefDefine(result);
+        assert(rd && "input ref port to instance is alive, but no driver?");
+        assert(isKnownAlive(rd.getSrc()));
+        result.replaceAllUsesWith(rd.getSrc());
+        ++numErasedOps;
+        rd.erase();
+        continue;


Maybe it is not assured that rd.getSrc() dominates all users of results?

Good call, thanks!

For this and other reasons, considering an op to bounce refs through that can be used in these situations -- replacing def of a ref that of course dominates its uses but the uses may not dominate each other (or indeed not all visible at any point). Something like %out, %in = firrtl.(ref.)pipe : !firrtl.ref<T>, or just a wire/wire-like, to plumb dataflow through. Such an op that guarantees (requires) a single driver is likely useful beyond references.

An op like that would hopefully avoid the need for the special casing done here and in ModuleInliner, or at least reduce it to producing the right sort of bounce declaration.

Looks like IMDCE presently assumes single-block, so one approach would be hoisting backwards_slice(driver), but that doesn't seem like a great approach or something to be bothering about in this pass.

Inliner has similar issues/concerns, and looks like it is intended to work with "when's" -- in some situations if there's input ref (same as the situation here w/IMDCE, for an example of this see [1]) it can produce invalid IR similarly re:dominance. Both are unreachable presently as we don't generate input reference ports.

One of a few candidate solutions...

[1] Example input (also crashes this IMDCE for reasons discussed): https://github.com/dtzSiFive/reftype-inputs/blob/ef6294d2c21063f5a26618e7e7b3d1a9c91896c5/inliner-refs.fir .

LLVM_DEBUG wrapped dominance check for now?

report fatal error if not already in same block and ordered legally. Shouldn't happen unless we enable input references prematurely or run IMDCE before ExpandWhen's. I did not want to introduce additional fragility to our passes re:hidden assumptions, but at least this has a guard. Handling it at all is only needed if there are input references that are used locally but dead through the instance -- which may be good to make illegal anyway.

dtzSiFive · 2023-03-20T21:38:54Z

Bikeshed: ref.connect instead of ref.define ?

I named it after the statement it's modelling that was added in FIRRTL 2.0, which seemed important at the time but seems less so now 👍. If it can be a FConnectLike it sure can be ref.connect (it's presently refconnect which has benefits but unless we rename the ref.* ops it seems like it should be also firrtl.ref.connect but 🤷). Don't feel strongly about this internally to CIRCT (vs really wanted to separate define from connect in the spec), thanks for the suggestion!

darthscsi

LGTM.

darthscsi · 2023-03-24T16:47:42Z

include/circt/Dialect/FIRRTL/FIRRTLOpInterfaces.td

@@ -340,6 +340,15 @@ def FConnectLike : OpInterface<"FConnectLike"> {
    "Value", "getDest", (ins)>,
    InterfaceMethod<"Return a source of connection.",
    "Value", "getSrc", (ins)>,
+    StaticInterfaceMethod<"Return connection behavior kind.",


Do we want a function on an interface or a trait? Might not matter.

darthscsi · 2023-03-24T16:51:49Z

lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp

+        auto getRefDefine = [](Value result) -> RefDefineOp {
+          for (auto *user : result.getUsers()) {
+            if (auto rd = dyn_cast<RefDefineOp>(user);
+                rd && rd.getDest() == result)
+              return rd;
+          }
+          return {};
+        };
+        auto rd = getRefDefine(result);
+        assert(rd && "input ref port to instance is alive, but no driver?");
+        assert(isKnownAlive(rd.getSrc()));
+        result.replaceAllUsesWith(rd.getSrc());
+        ++numErasedOps;
+        rd.erase();
+        continue;


LLVM_DEBUG wrapped dominance check for now?

dtzSiFive · 2023-03-28T22:41:06Z

Draft until dominance situation resolved (guard or appropriately modeled in IR). Thanks folks.

This reverts commit 8c93feb.

Restoring change previously introduced w/RefConnectOp PR, here RefDefineOp is the only op allowed to connect references (don't use connect when uninferred size).

Fixed!

…nnect, comment.

If refs were supported in strictconnect, 'flip' should matter for them just the same.

Good to check the output as well as not crashing.

Doesn't matter, mixed widths easier to debug.

dtzSiFive marked this pull request as ready for review March 13, 2023 16:41

dtzSiFive requested review from seldridge, darthscsi, prithayan and youngar March 13, 2023 18:58

darthscsi reviewed Mar 13, 2023

View reviewed changes

prithayan approved these changes Mar 13, 2023

View reviewed changes

dtzSiFive mentioned this pull request Mar 14, 2023

[FIRRTL][RefSub] Require source-flow operand, add verifier #4818

Closed

dtzSiFive force-pushed the feature/ref-define branch from 91d4c23 to 22ef929 Compare March 14, 2023 14:54

dtzSiFive mentioned this pull request Mar 14, 2023

[FIRRTL] Add RefCastOp, support ref path becoming more general #4825

Closed

uenoku reviewed Mar 15, 2023

View reviewed changes

darthscsi approved these changes Mar 24, 2023

View reviewed changes

dtzSiFive force-pushed the feature/ref-define branch from 79bcc7e to d51f613 Compare March 28, 2023 22:17

dtzSiFive marked this pull request as draft March 28, 2023 22:17

dtzSiFive added 12 commits March 29, 2023 10:36

Revert "[FIRRTL] RefConnect for references (llvm#4798)"

9c226a8

This reverts commit 8c93feb.

[FIRRTL] Add RefDefineOp.

66abdc6

[FIRRTL] Switch to emitConnect in FIRRTL Annotation methods.

5397f9a

Restoring change previously introduced w/RefConnectOp PR, here RefDefineOp is the only op allowed to connect references (don't use connect when uninferred size).

[FIRRTL] Add test for IMDCE refdefine change

88fd1b2

[FIRRTL] Simple tests for ExpandWhens + refs.

8a29ed8

[FIRRTL] Add test demonstrating issue 4334, fixed.

7a4817e

[FIRRTL] Add test demonstrating issue 3715.

609563c

Fixed!

[FIRRTL] Add test demonstrating issue 3713.

4c41a9d

[FIRRTL] FConnectLike: Use enum for behavior kinds.

4d0b8af

[ExpandWhens] Update for behavior kind, rename setConnect -> recordCo…

3737d55

…nnect, comment.

emitConnect: restore behavior, strictconnect for FIRRTL-only

002d3ea

[FIRRTL][LowerTypes] restore flip-of-ref behavior.

724d217

If refs were supported in strictconnect, 'flip' should matter for them just the same.

dtzSiFive added 10 commits March 29, 2023 10:36

[FIRRTL][LowerTypes] Clarify assertion reasoning.

e8f87a3

data-taps-xmr.fir: Add CHECK's for expected output.

6e33542

Good to check the output as well as not crashing.

data-taps-xmr: Use previous widths for smaller PR diff.

cbc1790

Doesn't matter, mixed widths easier to debug.

[FIRRTL] Tweak RefDefineOp description.

d401203

Fixup mention of 'assign' (-> 'define') in test

e443bcb

dom fail

006ef2e

[FIRParser] Allow define out of a block, now have define semantics.

0a3cba4

Add two XFAIL tests recording unsupported functionality / known issues.

95534af

test: ref.define

0e0b5de

update parse-errors error messages

ad88a39

dtzSiFive force-pushed the feature/ref-define branch from d51f613 to ad88a39 Compare March 29, 2023 20:40

dtzSiFive added 2 commits March 29, 2023 15:59

[FIRParser] note to self/others re:input probe dangers.

b687573

Add warnings to incorrect code in ModuleInliner, fatal_error in IMDCE.

47f45f3

dtzSiFive marked this pull request as ready for review March 29, 2023 21:21

[Inliner] Tweak comment.

5d01c99

dtzSiFive merged commit da6cb20 into llvm:main Mar 30, 2023

dtzSiFive deleted the feature/ref-define branch March 30, 2023 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIRRTL] Add RefDefineOp, disallow connecting references otherwise. #4812

[FIRRTL] Add RefDefineOp, disallow connecting references otherwise. #4812

dtzSiFive commented Mar 13, 2023 •

edited

Loading

dtzSiFive commented Mar 13, 2023

darthscsi Mar 13, 2023

dtzSiFive Mar 23, 2023

darthscsi commented Mar 13, 2023

prithayan left a comment

prithayan Mar 13, 2023

dtzSiFive Mar 14, 2023

uenoku Mar 15, 2023

dtzSiFive Mar 15, 2023 •

edited

Loading

darthscsi Mar 24, 2023

dtzSiFive Mar 29, 2023

dtzSiFive commented Mar 20, 2023

darthscsi left a comment

darthscsi Mar 24, 2023

darthscsi Mar 24, 2023

dtzSiFive commented Mar 28, 2023

[FIRRTL] Add RefDefineOp, disallow connecting references otherwise. #4812

[FIRRTL] Add RefDefineOp, disallow connecting references otherwise. #4812

Conversation

dtzSiFive commented Mar 13, 2023 • edited Loading

dtzSiFive commented Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darthscsi commented Mar 13, 2023

prithayan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtzSiFive Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtzSiFive commented Mar 20, 2023

darthscsi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtzSiFive commented Mar 28, 2023

dtzSiFive commented Mar 13, 2023 •

edited

Loading

dtzSiFive Mar 15, 2023 •

edited

Loading