llvm / circt

Circuit IR Compilers and Tools
https://circt.org
Other
1.57k stars 277 forks source link

[FIRRTL] 'Conditional' Analog attach #6468

Open fzi-hielscher opened 7 months ago

fzi-hielscher commented 7 months ago

I've been messing around with FIRRTL attach operations nested in WhenOp regions fully expecting them to be illegal. After all Chisel refuses to produce them. But, after careful reading of the spec, it appears to me that they should simply be unaffected by the condition and, in fact, this is what both SFC and firtool do:

foo.fir:

circuit Example :
  module Example :
    output a : Analog<1>
    output b : Analog<1>
    output c : Analog<1>
    output d : Analog<1>

    when UInt<1>("h1") :
      attach (a, b) 
    else :
      attach (c, d)
> firtool --verilog /tmp/foo.fir

// Generated by CIRCT unknown git version
module Example(
  inout a,
        b,
        c,
        d
);

  `ifdef SYNTHESIS
    assign a = b;
    assign b = a;
    assign c = d;
    assign d = c;
  `else  // SYNTHESIS
    `ifdef verilator
      `error "Verilator does not support alias and thus cannot arbitrarily connect bidirectional wires and ports"
      `error "Verilator does not support alias and thus cannot arbitrarily connect bidirectional wires and ports"
    `else  // verilator
      alias a = b;
      alias c = d;
    `endif // verilator
  `endif // SYNTHESIS
endmodule

But if we run the canonicalizer added in #6236 before the WhenOp is removed by ExpandWhens, the 'unreachable' attach is dropped too:

> .firtool --parse-only /tmp/foo.fir | circt-opt --canonicalize | firtool -format=mlir --verilog

// Generated by CIRCT unknown git version
module Example( // <stdin>:3:5
  inout a,      // <stdin>:3:32
        b,      // <stdin>:3:59
        c,      // <stdin>:3:86
        d       // <stdin>:3:113
);

  `ifdef SYNTHESIS      // <stdin>:4:7
    assign a = b;       // <stdin>:4:7
    assign b = a;       // <stdin>:4:7
  `else  // SYNTHESIS
    `ifdef verilator    // <stdin>:4:7
      `error "Verilator does not support alias and thus cannot arbitrarily connect bidirectional wires and ports"       // <stdin>:4:7
    `else  // verilator
      alias a = b;      // <stdin>:4:7
    `endif // verilator
  `endif // SYNTHESIS
endmodule

This is obviously a highly constructed example, but it did cause me some confusion. I have no clue under what circumstances we expect canonicalization to be run before ExpandWhens. But since it was added there appears to be a use case and I guess this technically violates the spec?

dtzSiFive commented 7 months ago

cc #4771