[FIRRTL] Missed optimization for name-preserved nodes

[youngar]

In the following example, I would expect both cover statements to be removed by constant propagation. Instead, only the first cover, which is fed by a wire, is removed.

circuit Foo:
  module X:
    output o : UInt<1>
    o <= UInt<1>(0)

  module Foo:
    input clock: Clock
    input i : UInt<1>

    inst x of X

    wire w : UInt<1>
    w <= and(x.o, i)
    cover(clock, w, UInt<1>(1), "")

    node n = and(x.o, i)
    cover(clock, n, UInt<1>(1), "")

module Foo(     // ./test.fir:6:10
  input clock,
        i);

  wire w = 1'h0;        // ./test.fir:10:5, :12:5
  wire n = 1'h0;        // ./test.fir:10:5, :16:5

  always @(posedge clock)       // ./test.fir:14:5
    cover(n);   // ./test.fir:16:5, :17:5
  X x ();       // ./test.fir:10:5
endmodule

The wire's cover is removed due to a canonicalizer which forwards constants "around" a name-preserved wire which doesn't exist for nodes:

firrtl.module @Foo(in %clock: !firrtl.clock, in %i: !firrtl.uint<1>) {
  %c0_ui1 = firrtl.constant 0 : !firrtl.uint<1>
  %c1_ui1 = firrtl.constant 1 : !firrtl.uint<1>
  %x_o = firrtl.instance x  @X(out o: !firrtl.uint<1>)
  %w = firrtl.wire   : !firrtl.uint<1>
  %0 = firrtl.and %c0_ui1, %i : (!firrtl.uint<1>, !firrtl.uint<1>) -> !firrtl.uint<1>
  firrtl.strictconnect %w, %0 : !firrtl.uint<1>
  firrtl.cover %clock, %w, %c1_ui1, ""  {eventControl = 0 : i32, isConcurrent = false}
  %n = firrtl.node  %0  : !firrtl.uint<1>
  firrtl.cover %clock, %n, %c1_ui1, ""  {eventControl = 0 : i32, isConcurrent = false}
}
// -----// IR Dump After Canonicalizer //----- //
firrtl.module @Foo(in %clock: !firrtl.clock, in %i: !firrtl.uint<1>) {
  %c0_ui1 = firrtl.constant 0 : !firrtl.uint<1>
  %c1_ui1 = firrtl.constant 1 : !firrtl.uint<1>
  %x_o = firrtl.instance x  @X(out o: !firrtl.uint<1>)
  %w = firrtl.wire   : !firrtl.uint<1>
  firrtl.strictconnect %w, %c0_ui1 : !firrtl.uint<1>
  firrtl.cover %clock, %c0_ui1, %c1_ui1, ""  {eventControl = 0 : i32, isConcurrent = false}
  %n = firrtl.node  %c0_ui1  : !firrtl.uint<1>
  firrtl.cover %clock, %n, %c1_ui1, ""  {eventControl = 0 : i32, isConcurrent = false}
}

The first cover statement is later removed after lowering to the SV dialect.

[darthscsi]

I think this might have been fixed here: https://github.com/llvm/circt/blob/31511a79f153c7229ce7121ec0c41ffa03229da0/lib/Dialect/FIRRTL/Transforms/IMConstProp.cpp#L631

[darthscsi]

maybe not. It does do the right thing, but canonicalize could be smarter.