[FIRRTL] Lower XMR assumes unique port names

[seldridge]

FIRRTL's LoweXMR pass will currently produce invalid IR if it is given a module which has duplicate port names.

firrtl.circuit "Foo" {
  firrtl.module @Foo(out %a: !firrtl.probe<uint<1>>, out %a0: !firrtl.probe<uint<1>>) attributes {convention = #firrtl<convention scalarized>, portNames = ["a", "a"]} {
    %b = firrtl.wire interesting_name : !firrtl.uint<1>
    %0 = firrtl.ref.send %b : !firrtl.uint<1>
    firrtl.ref.define %a, %0 : !firrtl.probe<uint<1>>
    %1 = firrtl.ref.send %b : !firrtl.uint<1>
    firrtl.ref.define %a0, %1 : !firrtl.probe<uint<1>>
  }
}

This produces:

# circt-opt Foo.mlir -firrtl-lower-xmr                                   
firrtl-snippets/issues/x.mlir:3:5: error: redefinition of symbol named 'ref_Foo_Foo_a'
    firrtl.module @Foo(out %a: !firrtl.probe<uint<1>>, out %a0: !firrtl.probe<uint<1>>) attributes {convention = #firrtl<convention scalarized>, portNames = ["a", "a"]} {
    ^
firrtl-snippets/issues/x.mlir:3:5: note: see current operation: "sv.macro.decl"() {sym_name = "ref_Foo_Foo_a"} : () -> ()
firrtl-snippets/issues/x.mlir:3:5: note: see existing symbol definition here

[dtzSiFive]

Generally it seems like either we be super careful re:ports and their names to preserve the original names (so ABI/convention based on FIRRTL source names is respected), or should record them early separately for use when exporting these things / assigning final names. I think presently we kinda do the former, with assumption no one will require the actual ABI/spec-promised port naming scheme until later on (?).

To keep in that spirit re:the-pipeline-is-the-how-naming-rules-are-enforced, maybe running a "these are what we promised they'd be named" pass done before/during/after LowerTypes? (and before anything is allowed to use their names in a way external users will see, such as this pass). While here this is an issue because of the ref ABI, the spec for general ABI says ports will have such-and-such name so if that's important we should be explicit and guarantee that behavior instead of having a pipeline that happens to do what's required ("combination of renaming/shuffling done by LowerTypes, and later name uniqueing will do what spec says it will").

---

If macro names were 1)separate from the symbols used to connect decl/def/refs and 2)able to take substitutions themselves, we could emit macros substituted over the final emitted port name (substitution from EV is good for ensuring they match what's produced, but doesn't directly ensure the final names will be what spec/ABI promise?), but not sure that's a reasonable idea :smile: . And that makes verifying macros don't collide a bit harder :upside_down_face: .

Is this reachable now or theoretically? Don't like IR causing crashes/verification failures, but if it's more "passes shouldn't produce invalid IR from valid IR" vs "this will cause us to do the wrong thing" changes how we might solve this at least in the short term.

Anyway, just some thoughts. WDYT?

[seldridge]

This is a great analysis!

I think it is easiest to add verification that the port names of public modules are unique. This is closest in spirit to how things work today. Why I think this is fine is because even if we hoist the names to a special area, there's no guarantee that a pass won't muck with them. I think any true solution here would require verification of the pass itself or across a pass. (I don't think this is necessary... Do you know of anything like this? Pass verifiers? Pass constraints?)

A simpler problem is that legal passes shouldn't change the circuit and top module name.

[dtzSiFive]

Oh, great idea! cc #3976 (#3968), although limiting to public modules makes sense!

Well I'm less worried about passes doing /wrong/ thing as much as ensuring the right thing is possible. Pass constraints is something we've talked quite a bit about (cc @youngar, @rwy7, @fabianschuiki) and would be great, especially for reasoning through our pipeline and its ordering + correctness.

Yep, requiring passes respect the ABI naming area is in same category as requiring they don't change circuit name and such :+1: . Not sure I'm earnestly suggesting we do the "stash the promised names to the side" necessarily but in some capacity if we need to ensure very specific names are used, we have everything we need to determine that at parse time (original signature + convention => names) so representing that somehow seems useful. Something to think about/discuss...

re:verifying ports have unique names, this requires passes to be good about this (not huge deal, only matters for ports don't need to unique w.r.t entire module body), but they shouldn't be adding ports that we don't already know the required names for (and if they do those ports can be named whatever we like?).

[seldridge]

Re: #3976, let's fix the passes so we can turn this on. It means that LowerTypes needs to work a little harder to get the port names right. However, that is way better than what I think is happening now---Export Verilog is determining FIRRTL-correct port names. 😬 This pain can likely be removed by making FModuleOp::insertPorts take care of the naming (including doing the right naming if you insert a name collision early).

[dtzSiFive]

Reachable from this input (maybe should be rejected?):

circuit InferProbe:
  module InferProbe:
    input x :  {a: UInt<3>, b: UInt<1>, c: UInt<5>} 
    output p : Probe<{a: UInt, b: UInt<1>, c: UInt}>
    output p_a : Probe<UInt>
    output p_b : Probe<UInt>
    output p_c : Probe<UInt>

    define p = probe(x)
    define p_a = p.a
    define p_b = p.b
    define p_c = p.c

[seldridge]

It's legal FIRRTL per the lowering ABI. output p takes precedence over the later ones. I don't see a reason to reject it given that we don't reject the same stuff for normal ports. It does mean that the ABI for reference defines needs to incorporate this information.