[SV] Why does `IfDefOp::thenRegion` enforce SSA dominance?

[rsetaluri]

Issue

The following code

hw.module @m(%a: i8, %b: i8, %clk: i1) ->  () {
    %0 = sv.reg {name = "reg0"} : !hw.inout<i8>
    sv.alwaysff(posedge %clk) {
        sv.passign %0, %1 : i8
    }
    %1 = comb.or %a, %b : i8
}

works and produces fine Verilog even though %1 is used before it is defined.

However, this code does not:

hw.module @m(%a: i8, %b: i8, %clk: i1) ->  () {
    sv.ifdef "C" {
    %0 = sv.reg {name = "reg0"} : !hw.inout<i8>
    sv.alwaysff(posedge %clk) {
        sv.passign %0, %1 : i8
    }
    %1 = comb.or %a, %b : i8
    }
}

resulting in:

<stdin>:5:9: error: operand #1 does not dominate this use
        sv.passign %0, %1 : i8
        ^
<stdin>:5:9: note: see current operation: "sv.passign"(%0, %1) : (!hw.inout<i8>, i8) -> ()
<stdin>:7:10: note: operand defined here (op in a parent region)
    %1 = comb.or %a, %b : i8
         ^

Note the only difference is that the IR is now wrapped in an sv.ifdef.

Proposal

I'm not extremely familiar with the semantics of regions, but my understanding is that in GraphRegions SSA dominance is not enforced, and hw::ModuleOp is such a type of region. From my understanding of Verilog ifdef semantics, there are no restrictions as to what can be inside such directives. So I would presume that we could allow the same inside sv::IfDefOp?

Happy to discuss the requirement/use-case further.

[uenoku]

I think it makes sense to make IfDefOp to have graph regions. It's inconsistent that there is difference between HWModule and IfDefOp.

[rsetaluri]

Thanks @uenoku. What would be the code change required to make that happen?

[uenoku]

You can modify the region kind by defining RegionKindInterface for the operation. For example, HWModule: https://github.com/llvm/circt/blob/2966699d0ef01baba92ab7b43436d24df587b8ff/include/circt/Dialect/HW/HWStructure.td#L41-L78

Adding RegionKindInterface to the operation (Line 41) and defining getRegionKind (Line 78) are necessary.
You might have to include "mlir/IR/RegionKindInterface.td" into tablegen.

[rsetaluri]

@uenoku - great I will try that code change. Should we also consider removing the NonProceduralOp attribute? It seems similarly overly restrictive.

[uenoku]

Should we also consider removing the NonProceduralOp attribute?

I don't think so. There are IfDefProceduralOp and IfDefOp operations, and this distinction makes sense to me.