[HW/SV] Add `hw.inout` elimination pass #5390

mortbopet · 2023-06-13T14:07:02Z

Adds a pass which analyses the module hierarchy to remove inout ports in favor of:

poking holes through the module hierarchy (explicit in/output ports)
raising sv.assign/sv.read_inout operations to the definition point of the hw.inout value.

This is unarbited, and only allows for multiple-readers, single-writer scenarios. Currently only supports sv.assign, but should probably also be able to raise sv.passign.

blakep-msft · 2023-06-14T20:04:25Z

lib/Dialect/SV/Transforms/HWRaiseInOutPorts.cpp

@@ -0,0 +1,194 @@
+//===- HWRaiseInOutPorts.cpp - Generator Callout Pass ---------------------===//


Looks good to me.

teqdruid

This is very similar to the port mapping that I do in ESI's lower ports pass:

circt/lib/Dialect/ESI/Passes/ESILowerPorts.cpp

Line 46 in 336b71f

class ChannelRewriter {

.

I put together a pretty-well (IMO) structured, pluggable infrastructure for solving this problem. It should be pulled out into a support library and made more generic. I think of it like a TypeConverter which handles creating both input and output ports simultaneously. Could you refactor it into a support lib and use it? (Refactor as first PR, use here as second.) We discussed this earlier today, but looking this over makes me think it's silly to re-invent the wheel given that these problems are identical.

lib/Dialect/SV/Transforms/HWRaiseInOutPorts.cpp

teqdruid · 2023-06-19T23:54:33Z

lib/Dialect/SV/Transforms/HWRaiseInOutPorts.cpp

+  // which share subcircuits.
+  llvm::DenseSet<InstanceGraphNode *> visited;
+  for (InstanceGraphNode *topModule : res.value()) {
+    // Visit the instance hierarchy in a depth-first manner, modifying child


nit: I don't think it's critical to do this DFS as your comment implies. I think this is only critical for "bubbling up" values, which this transform doesn't do.

Doing this DFS ensures that all module instance uses of an inout value has been converted before the current instance use.

E.g. say you have
m1 -> m2 -> m3

where both m3 and m2 reads an inout value defined in m1. If we don't do DFS, and we just randomly pick a module, we have to e.g. select m2, see that it also passes that inout value to other module instances, processes those first (which may bubble up read/writes to that hw.inout op), and then process m2... which in essence is a DFS traversal. So in my mind, it seems more logical, and safer/simpler, to just go ahead and do the DFS to begin with, ensuring the invariant that all module instance uses of an inout value have been converted before converting any given module.

lib/Dialect/SV/Transforms/HWRaiseInOutPorts.cpp

mortbopet · 2023-07-05T13:27:26Z

@teqdruid massively simplified by being based on the PortConverter 🥳.

Adds a pass which analyses the module hierarchy to remove `inout` ports in favor of: 1. poking holes through the module hierarchy (explicit in/output ports) 2. raising `sv.assign`/`sv.read_inout` operations to the definition point of the `hw.inout` value. This is unarbited, and only allows for multiple-readers, single-writer scenarios. Currently only supports `sv.assign`, but should probably also be able to raise `sv.passign`.

teqdruid

Thanks for this! Good to see that the port conversion infra helps here. My biggest problem is with the name.

teqdruid · 2023-07-11T21:13:04Z

include/circt/Dialect/SV/SVPasses.td

@@ -173,4 +173,17 @@ def HWExportModuleHierarchy : Pass<"hw-export-module-hierarchy",
  let dependentDialects = ["circt::sv::SVDialect"];
 }

+def HWRaiseInOutPorts : Pass<"hw-raise-inout-ports",


Shouldn't this be called something like "LowerInOutPorts"? Or EliminateInOutPorts? 'Breakout'? This doesn't seem like something which raises the level of abstraction, which is what the work 'raise' implies to me.

I'd favor Eliminate here. Raise was originally used since i was "raising" the sv.read/assign ops upwards in the instance hierarchy.

lib/Dialect/SV/Transforms/HWRaiseInOutPorts.cpp

teqdruid · 2023-07-11T21:17:00Z

lib/Dialect/SV/Transforms/HWRaiseInOutPorts.cpp

+  }
+
+  if (writers.size() > 1)
+    converter.getModule()->emitWarning()


I think this should be an error rather than a warning (and it should fail the pass). Hardware designers tend to ignore warnings, so I tend to prefer avoiding warnings, especially when they have this big of an effect on design correctness.

Agreed. Modified the infra slightly to allow PortConversions to provide a failable init function.

lib/Dialect/SV/Transforms/HWRaiseInOutPorts.cpp

teqdruid · 2023-07-11T21:24:27Z

lib/Dialect/SV/Transforms/HWRaiseInOutPorts.cpp

+void HWInOutPortConversion::mapOutputSignals(
+    OpBuilder &b, Operation *inst, Value instValue,
+    SmallVectorImpl<Value> &newOperands, ArrayRef<Backedge> newResults) {
+  llvm_unreachable("hw.inout outputs not yet supported");


Above you use an assert whereas here you use this. What should really be used? I think assert is more appropriate.

See below - will keep the assert since i nevertheless have to have something in the implementation.

teqdruid · 2023-07-11T21:26:54Z

lib/Dialect/SV/Transforms/HWRaiseInOutPorts.cpp

+public:
+  using PortConversionBuilder::PortConversionBuilder;
+  FailureOr<std::unique_ptr<PortConversion>> build(hw::PortInfo port) override {
+    if (port.direction == hw::PortDirection::INOUT)


Since we don't (yet) support output inouts, we should either ignore them or issue an error. Unsupported IR should never assert or segfault, as I think this results in.

Won't segfault; this code already implies that they're being ignored - see below. This, however, is probably more telling about how the HWModuleOp parses PortInfo, in that input hwinout has PortDirection::INOUT and output inout has PortDirection::OUTPUT. This in turn means that hw.inout gets handled as an untouched signal.
Added the below example as a test, mainly to serve as an early warning signal in case this logic changes.

hw.module @write(%a: !hw.inout<i42>) { %0 = hw.constant 0 : i42 sv.assign %a, %0 : i42 } hw.module @outputInout() -> (out : !hw.inout<i42>) { %0 = sv.wire : !hw.inout<i42> hw.output %0 : !hw.inout<i42> } hw.module @outputInoutDriver() { %0 = hw.instance "outputInout" @outputInout() -> (out : !hw.inout<i42>) hw.instance "write" @write(a : %0 : !hw.inout<i42>) -> () }

results in

hw.module @write() -> (a_wr: i42) { %c0_i42 = hw.constant 0 : i42 hw.output %c0_i42 : i42 } hw.module @outputInout() -> (out: !hw.inout<i42>) { %0 = sv.wire : !hw.inout<i42> hw.output %0 : !hw.inout<i42> } hw.module @outputInoutDriver() { %outputInout.out = hw.instance "outputInout" @outputInout() -> (out: !hw.inout<i42>) sv.assign %outputInout.out, %write.a_wr : i42 %write.a_wr = hw.instance "write" @write() -> (a_wr: i42) hw.output }

Yep. I forgot that confusing behavior. I'm hoping that'll change with lenharth's moduletype.

lib/Dialect/SV/Transforms/HWRaiseInOutPorts.cpp

teqdruid · 2023-07-11T21:29:18Z

lib/Dialect/SV/Transforms/HWRaiseInOutPorts.cpp

+  // Visit the instance hierarchy in a depth-first manner, modifying child
+  // modules and their ports before their parents.
+
+  // Doing this DFS ensures that all module instance uses of an inout value has


Do we need to do some cycle detection here?

Not that i'm aware of, post-order iteration should only visits each node once, even in the presence of cycles.

teqdruid · 2023-07-11T21:31:02Z

lib/Dialect/SV/Transforms/HWRaiseInOutPorts.cpp

+  llvm::DenseSet<InstanceGraphNode *> visited;
+  auto res = instanceGraph.getInferredTopLevelNodes();
+
+  // Visit the instance hierarchy in a depth-first manner, modifying child


Not for this PR, but this feels like something which could be pulled out. Perhaps a part of the PortConversion framework... IIRC the ESI pass does something like this as well.

that, or some kind of boring utility library (global/hierarchy modification) which can be coupled together with PortConvesion (local changes).

teqdruid · 2023-07-11T21:34:23Z

lib/Dialect/SV/Transforms/HWRaiseInOutPorts.cpp

+
+  if (writers.size() > 1)
+    converter.getModule()->emitWarning()
+        << "Multiple writers of inout port " << port.name


Not for this PR, but what is the behavior of multiple assignments to an inout variable declared in the same module? Is it defined? What about PAssignOp? Perhaps we should (in the future) try to emulate that behavior.

what is the behavior of multiple assignments to an inout variable declared in the same module? Is it defined? What about PAssignOp?

IAFAIK, CIRCT won't complain but you're going to have a problem during synthesis given the multiple writers.
And yea, as mentioned in the PR, PAssignOp is explicitly not covered by this pass.

Adds a pass which analyses the module hierarchy to eliminate `inout` ports in favor of: 1. poking holes through the module hierarchy (explicit in/output ports) 2. raising `sv.assign`/`sv.read_inout` operations to the definition point of the `hw.inout` value. This is unarbited, and only allows for multiple-readers, single-writer scenarios. Currently only supports `sv.assign`, as well as `hw.inout` input ports.

mortbopet requested review from mikeurbach and teqdruid June 13, 2023 14:07

mortbopet force-pushed the dev/mpetersen/read_write_plumbing branch 2 times, most recently from 6f8d234 to 4cf599c Compare June 13, 2023 14:09

blakep-msft reviewed Jun 14, 2023

View reviewed changes

teqdruid reviewed Jun 20, 2023

View reviewed changes

mortbopet mentioned this pull request Jul 3, 2023

[Support/ESI] Add generic port conversion utility #5533

Merged

mortbopet changed the title ~~[HW/SV] Add inout raising pass~~ [HW/SV] Add inout conversion pass Jul 5, 2023

mortbopet force-pushed the dev/mpetersen/read_write_plumbing branch 2 times, most recently from 1db1c2d to 256a7e2 Compare July 5, 2023 13:26

mortbopet marked this pull request as ready for review July 5, 2023 13:27

mortbopet added 3 commits July 11, 2023 15:25

rebase

7d38f64

rebase

e63c9d8

mortbopet force-pushed the dev/mpetersen/read_write_plumbing branch from 256a7e2 to e63c9d8 Compare July 11, 2023 13:50

teqdruid approved these changes Jul 11, 2023

View reviewed changes

mortbopet added 2 commits July 12, 2023 07:40

Failable PortConverter

2b322e9

rename

29b9e97

mortbopet changed the title ~~[HW/SV] Add inout conversion pass~~ [HW/SV] Add hw.inout elimination pass Jul 12, 2023

mortbopet merged commit 0e47744 into main Jul 12, 2023

darthscsi deleted the dev/mpetersen/read_write_plumbing branch June 4, 2024 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HW/SV] Add `hw.inout` elimination pass #5390

[HW/SV] Add `hw.inout` elimination pass #5390

mortbopet commented Jun 13, 2023

blakep-msft Jun 14, 2023

teqdruid left a comment

teqdruid Jun 19, 2023

mortbopet Jun 20, 2023

mortbopet commented Jul 5, 2023

teqdruid left a comment

teqdruid Jul 11, 2023

mortbopet Jul 12, 2023

teqdruid Jul 11, 2023

mortbopet Jul 12, 2023

teqdruid Jul 11, 2023

mortbopet Jul 12, 2023

teqdruid Jul 11, 2023

mortbopet Jul 12, 2023 •

edited

Loading

teqdruid Jul 12, 2023

teqdruid Jul 11, 2023

mortbopet Jul 12, 2023

teqdruid Jul 11, 2023

mortbopet Jul 12, 2023

teqdruid Jul 11, 2023

mortbopet Jul 12, 2023

		@@ -0,0 +1,194 @@
		//===- HWRaiseInOutPorts.cpp - Generator Callout Pass ---------------------===//

[HW/SV] Add hw.inout elimination pass #5390

[HW/SV] Add hw.inout elimination pass #5390

Conversation

mortbopet commented Jun 13, 2023

Choose a reason for hiding this comment

teqdruid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortbopet commented Jul 5, 2023

teqdruid 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

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

mortbopet Jul 12, 2023 • edited Loading

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

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

[HW/SV] Add `hw.inout` elimination pass #5390

[HW/SV] Add `hw.inout` elimination pass #5390

mortbopet Jul 12, 2023 •

edited

Loading