[FIRRTL] Add Domain Info to WireOp

[seldridge]

Currently, there's no way to verify the legality of firrtl.domain.define when a WireOp is involved. E.g., the following is trivially an error after domain inference, but there is no way to actually enshrine the legality/illegality in the IR:

firrtl.circuit "Foo" {
  firrtl.domain @ClockDomain
  firrtl.domain @PowerDomain
  firrtl.module @Foo(
    in %in: !firrtl.domain of @ClockDomain,
    out %out: !firrtl.domain of @PowerDomain
  ) {
    %w = firrtl.wire : !firrtl.domain // We don't have syntax or storage to mark the domain that %w is in.
    firrtl.domain.define %w, %in
    firrtl.domain.define %out, %w
  }
}

Modify FIRRTL's WireOp to include optional domain information, i.e., a DominInfoAttr. Update passes to support this.

[MrRoy09]

Hi, I've taken a look at this and implemented the following changes:

• Added OptionalAttr<DomainInfoAttr>:$domainInfo to WireOp's argument list in FIRRTLDeclarations.td
• Updated all 5 WireOp builder methods to accept and set the domainInfo parameter
• Updated getDomainTypeName() in FIRRTLOps.cpp to handle WireOp
• Removed the verification workaround in DomainDefineOp::verify()
• Updated all WireOp::create() call sites in the following files to pass the domainInfo parameter:
  • InferResets.cpp
  • LowerOpenAggs.cpp
  • ModuleInliner.cpp

I have tested these changes locally and they work as expected. I'll create a PR soon.

Question: I couldn't find any code for performing domain inference. I'm assuming that's not in scope for this issue and would be handled separately?

[seldridge]

Question: I couldn't find any code for performing domain inference. I'm assuming that's not in scope for this issue and would be handled separately?

That's currently PR'd here: #9106

I'm currently trying to get all of this stood up together and I may prefer to have #9106 land first. However, that's not a reason to not open a PR!

Thanks for looking at this.

[MrRoy09]

@seldridge I’m continuing work on adding DomainInfoAttr to WireOp and I am working on updating the transformation passes. I have a couple of questions -

• Is WireOp intended to support per-field domains (e.g., [[0], [1]] for a bundle where field_a is in dom0 and field_b is in dom1)?
• If the entire wire/bundle is always expected to share the same domain set, would it be better to simplify WireOp to use a flat array like Module Ports instead of DomainInfoAttr

[seldridge]

Is WireOp intended to support per-field domains (e.g., [[0], [1]] for a bundle where field_a is in dom0 and field_b is in dom1)?

Not initially, but eventually. We will probably need to have this for ports eventually. However, I've been avoiding tackling that, just yet. It's fine if WireOp initially only has the same capabilities as individual ports (either they are a domain or they are in a set of domains).

If the entire wire/bundle is always expected to share the same domain set, would it be better to simplify WireOp to use a flat array like Module Ports instead of DomainInfoAttr

There's two situations that I was originally trying to support:

wire a : Domain of ClockDomain
wire b : UInt<1> domains [a]

This was my original reasoning of DomainInfoAttr as the storage is either a FlatSymbolRefAttr in the first case or an array of domains. However, this doesn't work nicely when an array as this really has to use SSA values in the wire b case.

How about this: just an Optional<FlatSymbolRefAttr> which is only set if the type of the wire is a domain type? No support for fields, no storage for non-domain types?