HandshakeToFIRRTL should respect FIRRTL requiring classic SSA def-before-use

[mikeurbach]

The following psuedo-IR can be produced by HandshakeToFIRRTL, passes the verifier, but will crash the LowerTypes pass:

%0 = firrtl.instance ...
firrtl.connect %0, %1
%1 = firrtl.instance ...

The actual test case I have is quite complex, and I'm trying to reduce it to something we can realistically put in a unit test.

The solution quick fix seems pretty clear: instead of processing every operation in order in the LowerTypes runOnOperation function, just track the connects to process, and take a second pass to actually process them. I've implemented this already, and will share a PR once I've reduced a reasonable test case.

[seldridge]

Interesting... Is that supposed to be legal given that it's a use before def of an SSA value?

The SFC disallows use before def like this:

circuit Foo:
  module Foo:
    input a: UInt<1>
    output b: UInt<1>

    x <= a
    wire x: UInt<1>
    b <= x

firrtl -i Foo.fir
# Exception in thread "main" firrtl.passes.CheckHighFormLike$UndeclaredReferenceException: : [module Foo] Reference x is not declared.

But, interestingly, CIRCT will allow the equivalent MLIR input. 🤔

My gut is that this should be a hard error and we should fix this in HandshakeToFIRRTL so it doesn't emit this style of code.

[mikeurbach]

Since Handshake supports Graph regions, it is natural to use values before they are defined. The HandshakeToFIRRTL pass just loops over the ops in order as it generates FIRRTL, so situations like this can arise. I guess we could have HandshakeToFIRRTL generate all the connects at the end, kind of like what was discussed here in the context of RTL/SV: #438.

It's not up to me if it should be a requirement that FIRRTL modules enforce definitions before use, so I'll leave it to the FIRRTL owners. That said, the quick fix for this within LowerTypes is simple, and I've already implemented it, so I'll make a PR anyway. If you ultimately decide this IR shouldn't be valid, I can update the HandshakeToFIRRTL pass to resolve the impedance mismatch between Graph and SSA regions.

[lattner]

I don't think the firrtl dialect should support this, it should require classic def-before-use SSA-style dominance. The RTL dialect should support more flexible graph-style constructs. LowerToRTL (as one example) won't work with graph style FIRRTL IR.

[lattner]

This is easy enough to fix, we should remove this from FModuleOp:

  // Implement RegionKindInterface.
    static RegionKind getRegionKind(unsigned index) { return RegionKind::Graph;}

And remove the RegionKindInterface trait.

Rationale: FIRRTL is really a part of the Chisel stack, and it doesn't need graph support. We should implement most of the interesting logic in CIRCT on the RTL dialect, and keep the FIRRTL lowering stuff as simple as possible. I'd recommend Handshake lowering to eventually move to targeting RTL directly as it gets more mature.

[mikeurbach]

Sounds good to me. This is really a HandshakeToFIRRTL issue then.

[lattner]

Thanks Mike, I changed the FIRRTL dialect to correctly enforce this in bf56ef1. This broke HandshakeToFIRRTL/test_load.mlir which I xfailed. I looked into it, but didn't see an obvious solution. I hope that is ok, if not, lemme know and I'll be happy to revert this for now.

[hanchenye]

Thanks @lattner, the change in bf56ef1 makes sense to me. I just created a patch for fixing the failed test case.

[hanchenye]

Done in #537

[lattner]

thx!