[FIRRTL] Type Canonicalization May Require Connect Canonicalization

[seldridge]

Consider the following two circuits that the Scala FIRRTL Compiler (SFC) treats as equivalent:

circuit Foo1 :
  module Foo1 :
    input a : { a : UInt<1>}
    output b : { a : UInt<1>}
    b <= a

Here, the inputs are flipped and the connection order is flipped:

circuit Foo2 :
  module Foo2 :
    output a : { flip a : UInt<1>}
    input b : { flip a : UInt<1>}
    a <= b

Either a connect (<=) or partial connect (<-) can be used.

This is problematic, because MLIR FIRRTL Dialect type canonicalization changes the types of the ports from Foo2 into those of Foo1. However, the connect is untouched.

I'm not 100% sure how to handle this, but this may require tracking when type canonicalization results in an outer flip and then optionally updating the LHS and RHS.

Simply swapping the connection order so it works is differently problematic because this will cause erroneous circuits to be accepted, e.g., a <= b would then work in Foo1 and b <= a would work in Foo2 when these are rejected by the SFC.

[mikeurbach]

I think this came up when flattening bundle types, specifically when generating connects for each field. I added this logic to check if the field was flipped, and swap the LHS and RHS accordingly:

circt/lib/Dialect/FIRRTL/Transforms/LowerTypes.cpp

Lines 327 to 330 in 92571ec

	if (newDest.getType().isa<FlipType>())
	builder->create<ConnectOp>(newDest, newSrc);
	else
	builder->create<ConnectOp>(newSrc, newDest);

That seemed to work, but I wasn't sure if something more robust would be needed...

[seldridge]

This will accept more circuits than we want, but I've already advocated for falsely accepting circuits as being preferable to falsely rejecting circuits. Your solution seems fine right now, @mikeurbach. 👍

As a note: seeing any usages of aggregates using <= or <- is already pretty rare in Chisel-generated FIRRTL IR. For any modern Chisel, connections are expanded at compile time into constituent connections (so this problem doesn't come up). However, legacy code will emit bulk connections as <- and any combination of connections is allowable per the spec. For documentation purposes, the following snippet shows the current Chisel behavior:

• https://scastie.scala-lang.org/DeYya2FATVidrY6Kba6spA

[seldridge]

The other underlying problem here is that if a is a wire or an input, then a <= b means "drive a with the output b" (which is legal) or is an illegal connection.

What I seem to be running into is that the FIRRTL spec is wrapping up types, kinds, and flows into the type system while we only capture the former. I've thought about using the RTL dialect's inout type on wires and registers to try and capture that these have different semantics for the same types.

[lattner]

Yes, I agree that port canonicalization into types is going to be problematic for the whole notion of 'flow' in section 8 of the firrtl spec. Your examples are great:

circuit Foo2 :
  module Foo2 :
    output a : { flip a : UInt<1>}
    input b : { flip a : UInt<1>}
    a <= b

I don't think that flip canonicalization is the actual problem, it is that we model output as flip that is the issue. I think we have two options:

1. Add a notion of input and output to firrtl.module ports.
2. Use a more loose definition of flow in the face of bundles.

I'm in favor of #2 unless there is a downside.

[lattner]

should this stay open?

[seldridge]

I think we can close this. We've been pushing in the #2 direction and it's been working fine.

If I have a clear situations that requires us to flip a connect, I'll re-open this.