[FIRRTL] Disabling an enabled layer on the main module can trip the verifier

[seldridge]

Consider the following circuit:

FIRRTL version 5.2.0
circuit Foo:
  layer A, bind:
  public module Foo enablelayer A:

If this is compiled with the option to disable layer A, then the main module is deleted and the circuit is invalid:

# firtool Foo.fir -disable-layers=A
Foo.fir:2:1: error: 'firrtl.circuit' op does not contain module with same name as circuit
circuit Foo:
^
Foo.fir:2:1: note: see current operation: 
"firrtl.circuit"() <{annotations = [], name = "Foo"}> ({
^bb0:
}) : () -> ()

If the circuit continues to require that there exists a main module in it with the same name, then this should produce a better error. However, I think the better approach is likely to not require a main module.

[dtzSiFive]

Yeah this was encountered almost immediately after that feature was added. The error message isn't ideal for user-facing, but otherwise is correct and working as expected for the current FIRRTL specification.

Sounds like this is a FIRRTL specification change request filed as a CIRCT issue, yes?
If we can specialize to a circuit to one with no main module (and allow that) probably we should allow that as the original input as well (and drop the verifier). This sort of consistency is important. For additional concrete reasons, consider what FIRRTL export should do in this case, so on.

Once that's accomplished, there's no issue here.

Sound good?

[seldridge]

Yeah, removing the requirement on the FIRRTL spec that the circuit has a main module as opposed to being a container of public modules is the right approach. This makes sense to me.

The error message isn't ideal for user-facing, but otherwise is correct and working as expected for the current FIRRTL specification.

I don't really agree with this as verifiers are only supposed to catch user errors if they originate from parsing. After that, any verifier error after a pass is a bug in CIRCT under the assumption that passes that consume legal IR must produce legal IR.

Having the pass signalPassFailure when it sees this, with a good error message, is totally fine though. We've had a lot of places where this kind of error is the best we can do (e.g., lots of old Grand Central errors looked like this). We can also move this to an early parser error / circuit verifier. It's relatively trivial to see that the parsed circuit is illegal given the choice to put the disabled layers into the IR 🎊:

  firrtl.circuit "Foo" attributes {disable_layers = [@A]} {
    firrtl.layer @A bind {
    }
    firrtl.module @Foo() attributes {convention = #firrtl<convention scalarized>, layers = [@A]} {
    }
  }

[dtzSiFive]

I don't really agree with this as verifiers are only supposed to catch user errors if they originate from parsing. After that, any verifier error after a pass is a bug in CIRCT under the assumption that passes that consume legal IR must produce legal IR.

Err, absolutely/agreed. Modulo policy on parser generating errors in terms of user code vs verifier (usually in terms of MLIR operations/types/values).

My point should have been that producing an error seems appropriate in this situation but maybe could be nicer given it's reachable from user input (and the distance Chisel users have from the firtool invocation, possibly without any visibility regarding layers and their specialization, leading to confusion). Having the pass check for this as you mention is indeed a possibility if we want to go this route 👍 .