[FIRRTL] Analog Module Connection Generates Unnecessary Alias/Assign

[seldridge]

The following circuit generates an alias/attach for an analog connection from an instance to another instance. However, anytime we can avoid generating an alias/attach means that more tools will accept it. This should just generate a wire.

circuit Foo:
  extmodule Bar:
    output a: Analog<1>
  extmodule Baz:
    input a: Analog<1>
  module Foo:

    inst bar of Bar
    inst baz of Baz

    attach(baz.a, bar.a)

This produces with the MFC:

module Foo();
  wire _a_wire;
  wire _a_wire_0;
  `ifdef SYNTHESIS
    assign _a_wire_0 = _a_wire;
    assign _a_wire = _a_wire_0;
  `else  // SYNTHESIS
    `ifdef verilator
      `error "Verilator does not support alias and thus cannot arbitrarily connect bidirectional wires and ports"
    `else  // verilator
      alias _a_wire_0 = _a_wire;
    `endif // verilator
  `endif // SYNTHESIS
  Bar bar (
    .a (_a_wire)
  );
  Baz baz (
    .a (_a_wire_0)
  );
endmodule

SFC produces:

module Foo(
);
  wire  _GEN_0;
  Bar bar (
    .a(_GEN_0)
  );
  Baz baz (
    .a(_GEN_0)
  );
endmodule

[abejgonzalez]

What's the status of this issue? Is this potentially going to be worked on sometime in the future?

[seldridge]

This isn't currently prioritized. There's currently special casing inside the LowerToHW conversion to match on "pass analog up" and "pass analog down" case. This would need to be extended to also understand that U-turns are allowed (both pass-up and pass-down is okay).

Do you all have any bandwidth to help out?

[abejgonzalez]

Unfortunately not at the moment. Do you expect the work to be hard (any estimates of effort v.s. time)(subjectively, how hard is the ramp-up time to learn CIRCT FIRTOOL internals)?

[seldridge]

I looked at this before and it's a tiny bit tedious, but not bad. The place where this should get handled is in the LowerToHW conversion. There is logic inside this function that is special casing situations where an attach will be effectively ignored if it has a single use. This catches the case of port-to-instance connections, but needs to be extended to handle cases where there is a single value that can be used. I looked at this a while ago and it seemed like it would require more refactoring than I had hoped.

Generally, ramping up on CIRCT internals is not bad, especially if you're doing something compartmentalized, e.g., modifications to an existing pass up to writing a single pass. Anything more broad reaching requires more context. Having C++ experience helps, though LLVM/MLIR/CIRCT development is closer to "C++ on rails". You are heavily following LLVM coding conventions (e.g., resource-allocation-is-initialization (RAII) is used everywhere / you will almost never manually heap allocate something), you use LLVM internal libraries in place of normal ones (llvm::SmallVector over std::vector), and there are myriad convenience utilities that make passes easy to write (e.g., LLVM has an "RAUW" operation that "replaces all uses with" which was somewhat mind bending coming from SFC development). We usually see ~5 days for a "good first issue" PR for someone with no experience. The actual change is probably 0.5--2 days for a seasoned developer.

[tymcauley]

Just looked into this a bit, and it looks like the function that's generating this ifdef statement is in FIRRTLLowering::visitStmt(AttachOp). The function you linked is only called by FIRRTLModuleLowering::lowerModuleBody if the analog value in question is a module port. Indeed, if we modify the above test circuit to connect this analog to a port, then everything looks great:

circuit Foo:
  extmodule Bar:
    output a: Analog<1>
  extmodule Baz:
    input a: Analog<1>
  module Foo:
    output a: Analog<1>

    inst bar of Bar
    inst baz of Baz

    attach(baz.a, bar.a)
    attach(a, bar.a)

Produces

module Foo(
  inout a);

  Bar bar (
    .a (a)
  );
  Baz baz (
    .a (a)
  );
endmodule

But once the analog signal is an internal-only wire, then we fall through to the FIRRTLModuleLowering::lowerModuleOperations call, which invokes the FIRRTLVisitor, which calls FIRRTLLowering::visitStmt(AttachOp) on this attach statement.

I've got a question about that function: what are the cases where we want that ifdef block generated from an input FIRRTL file? It seems like a pretty big departure from the Verilog generated by the SFC. Could we replace that call to addToIfDefBlock with something like this?

auto v0 = inoutValues.front();
for (auto v : inoutValues) {  
  if (v == v0) {              
    continue;                 
  }
  v.replaceAllUsesWith(v0);   
}

I'm quite early in my writing-code-for-CIRCT education, so I recognize I might be way off-base on this solution. Also, if it's easier to discuss there, I'm on the LLVM discord (tynan-nb#2561). Thanks for any feedback!

[seldridge]

I've got a question about that function: what are the cases where we want that ifdef block generated from an input FIRRTL file?

We want to only generate the ifdef block when we can't generate this in any other way. I think the situation where that comes up is if we ever have a u-turn from a module to a module port. I don't think there is a Verilog representation for the following that doesn't require the ifdef:

module Foo:
  input a: Analog<1>
  output b: Analog<1>
  attach(b, a)

Anything else is okay. If it is a u-turn from an instance port to one or more other instance ports, then that's just a dummy wire. If this is used by exactly one module port, then the module port name is used instead of the dummy wire.

[tymcauley]

Understood, and I see the SFC emits the same code in that case. Is this more what you had in mind?

// If all operands of the attach are internal to this module (none of them
// are ports), then they can all be replaced with a single wire, and we can
// delete the attach op.
bool isAttachInternalOnly = true;                                          
for (auto v : inoutValues) {
  if (isa<BlockArgument>(v)) {
    isAttachInternalOnly = false;
    break;
  }
}                                                                          
                                                                           
if (isAttachInternalOnly) {                                                
  auto v0 = inoutValues.front();
  for (auto v : inoutValues) {
    if (v == v0) {                                                         
      continue;                                                            
    }
    v.replaceAllUsesWith(v0);
  }                                                                        
  return success();
}

// If the attach operands contain a port, then we can't do anything to     
// simplify the attach operation.
addToIfDefBlock(                                                           
  ...