[FIRRTL][LowerToHW] use-before-def with inline layer

[seldridge]

The following fails to compile due to a use-before-def created by LowerToHW:

FIRRTL version 5.1.0
circuit Foo :
  layer Verif, inline :

  public module Foo :
    input clock : Clock
    input reset : UInt<1>
    input enq : { flip ready : UInt<1>, valid : UInt<1>, bits : UInt<8>}
    output deq : { flip ready : UInt<1>, valid : UInt<1>, bits : UInt<8>}

    connect deq, enq
    wire valid : UInt<1>
    wire valid_rwprobe : RWProbe<UInt<1>, Verif>
    define valid_rwprobe = rwprobe(valid)
    connect valid, enq.valid
    connect deq.valid, valid
    skip
    layerblock Verif :
      regreset go_c_value : UInt<2>, clock, reset, UInt<2>(0h0)
      wire go : UInt<1>
      connect go, UInt<1>(0h0)
      when UInt<1>(0h0) :
        connect go_c_value, UInt<1>(0h0)
      else :
        when UInt<1>(0h1) :
          node go_wrap_wrap = eq(go_c_value, UInt<2>(0h3))
          node _go_wrap_value_T = add(go_c_value, UInt<1>(0h1))
          node _go_wrap_value_T_1 = tail(_go_wrap_value_T, 1)
          connect go_c_value, _go_wrap_value_T_1
          connect go, go_wrap_wrap
      when go :
        node has_been_reset = intrinsic(circt_has_been_reset : UInt<1>, clock, reset)
        node disable = eq(has_been_reset, UInt<1>(0h0))
        node _T = eq(disable, UInt<1>(0h0))
        force(clock, _T, valid_rwprobe, UInt<1>(0h0))

This is producing:

Foo.fir:19:7: error: operand #0 does not dominate this use
      regreset go_c_value : UInt<2>, clock, reset, UInt<2>(0h0)
      ^
Foo.fir:19:7: note: see current operation: %8 = "seq.firreg"(%14, %arg0, %arg1, %2) <{name = "go_c_value"}> {firrtl.random_init_start = 0 : ui64} : (i2, !seq.clock, i1, i2) -> i2
JackRWProbe.fir:28:37: note: operand defined here (op in the same block)
          node _go_wrap_value_T_1 = tail(_go_wrap_value_T, 1)

MLIR after failure:

#loc = loc("JackRWProbe.fir":8:11)
#loc1 = loc("JackRWProbe.fir":9:12)
"builtin.module"() ({
  "sv.macro.decl"() <{sym_name = "SYNTHESIS"}> : () -> ()
  "hw.hierpath"() <{namepath = [#hw.innerNameRef<@Foo::@sym>], sym_name = "xmrPath"}> {sym_visibility = "private"} : () -> ()
  "sv.macro.decl"() <{sym_name = "layer_Foo$Verif"}> : () -> ()
  "hw.module"() <{comment = "", module_type = !hw.modty<input clock : !seq.clock, input reset : i1, output enq_ready : i1, input enq_valid : i1, input enq_bits : i8, input deq_ready : i1, output deq_valid : i1, output deq_bits : i8>, parameters = [], per_port_attrs = [], result_locs = [#loc, #loc1, #loc1], sym_name = "Foo"}> ({
  ^bb0(%arg1: !seq.clock, %arg2: i1, %arg3: i1, %arg4: i8, %arg5: i1):
    %0 = "seq.from_clock"(%arg1) : (!seq.clock) -> i1
    %1 = "hw.constant"() <{value = -1 : i2}> : () -> i2
    %2 = "hw.constant"() <{value = 0 : i2}> : () -> i2
    %3 = "hw.constant"() <{value = true}> : () -> i1
    %4 = "hw.constant"() <{value = false}> : () -> i1
    %5 = "hw.wire"(%6) <{name = "deq_valid"}> : (i1) -> i1
    %6 = "hw.wire"(%arg3) <{inner_sym = #hw<innerSym@sym>, name = "valid"}> : (i1) -> i1
    "sv.ifdef"() <{cond = #sv<macro.ident @layer_Foo$Verif>}> ({
      %7 = "sv.xmr.ref"() <{ref = @xmrPath, verbatimSuffix = ""}> : () -> !hw.inout<i1>
      %8 = "seq.firreg"(%14, %arg1, %arg2, %2) <{name = "go_c_value"}> {firrtl.random_init_start = 0 : ui64} : (i2, !seq.clock, i1, i2) -> i2
      %9 = "hw.wire"(%10) <{name = "go"}> : (i1) -> i1
      %10 = "comb.icmp"(%8, %1) <{predicate = 0 : i64, twoState}> {sv.namehint = "go_wrap_wrap"} : (i2, i2) -> i1
      %11 = "comb.concat"(%4, %8) : (i1, i2) -> i3
      %12 = "comb.concat"(%2, %3) : (i2, i1) -> i3
      %13 = "comb.add"(%11, %12) <{twoState}> {sv.namehint = "_go_wrap_value_T"} : (i3, i3) -> i3
      %14 = "comb.extract"(%13) <{lowBit = 0 : i32}> {sv.namehint = "_go_wrap_value_T_1"} : (i3) -> i2
      %15 = "verif.has_been_reset"(%0, %arg2) <{async = false}> {sv.namehint = "has_been_reset"} : (i1, i1) -> i1
      %16 = "comb.and"(%9, %15) <{twoState}> : (i1, i1) -> i1
      "sv.ifdef"() <{cond = #sv<macro.ident @SYNTHESIS>}> ({
      ^bb0:
      }, {
        "sv.always"(%0) <{events = [0 : i32]}> ({
          "sv.if"(%16) ({
            "sv.force"(%7, %4) : (!hw.inout<i1>, i1) -> ()
          }, {
          }) : (i1) -> ()
        }) : (i1) -> ()
      }) : () -> ()
    }, {
    }) : () -> ()
    "hw.output"(%arg5, %5, %arg4) : (i1, i1, i8) -> ()
  }) : () -> ()
  "om.class"() <{fieldNames = [], fieldTypes = {}, formalParamNames = ["basepath"], sym_name = "Foo_Class"}> ({
  ^bb0(%arg0: !om.basepath):
    "om.class.fields"() <{field_locs = []}> : () -> ()
  }) : () -> ()
}) : () -> ()

[dtzSiFive]

Looks unrelated to probes or force, consider:

FIRRTL version 5.1.0
circuit Foo :
  layer Verif, inline :

  extmodule E :
    input x : UInt<1>

  public module Foo :
    input clock : Clock
    input reset : UInt<1>

    layerblock Verif :
      regreset go_c_value : UInt<2>, clock, reset, UInt<2>(0h0)
      wire go : UInt<1>
      connect go, UInt<1>(0h0)
      when UInt<1>(0h0) :
        connect go_c_value, UInt<1>(0h0)
      else :
        when UInt<1>(0h1) :
          node go_wrap_wrap = eq(go_c_value, UInt<2>(0h3))
          node _go_wrap_value_T = add(go_c_value, UInt<1>(0h1))
          node _go_wrap_value_T_1 = tail(_go_wrap_value_T, 1)
          connect go_c_value, _go_wrap_value_T_1
          connect go, go_wrap_wrap
      inst e of E
      connect e.x, UInt<1>(0)
      when go :
        connect e.x, go

Which results in:

issue-8461-mod-3.fir:13:7: error: operand #0 does not dominate this use
      regreset go_c_value : UInt<2>, clock, reset, UInt<2>(0h0)
      ^
issue-8461-mod-3.fir:13:7: note: see current operation: %4 = "seq.firreg"(%10, %arg0, %arg1, %1) <{name = "go_c_value"}> {firrtl.random_init_start = 0 : ui64} : (i2, !seq.clock, i1, i2) -> i2
issue-8461-mod-3.fir:22:37: note: operand defined here (op in the same block)
          node _go_wrap_value_T_1 = tail(_go_wrap_value_T, 1)
                                    ^
// -----// IR Dump After LowerFIRRTLToHW Failed (lower-firrtl-to-hw) //----- //
#loc = loc("issue-8461-mod-3.fir":6:11)
"builtin.module"() ({
  "sv.macro.decl"() <{sym_name = "layer_Foo$Verif"}> : () -> ()
  "hw.module.extern"() <{module_type = !hw.modty<input x : i1>, parameters = [], per_port_attrs = [], port_locs = [#loc], sym_name = "E"}> ({
  }) {comment = "", sym_visibility = "private"} : () -> ()
  "hw.module"() <{comment = "", module_type = !hw.modty<input clock : !seq.clock, input reset : i1>, parameters = [], per_port_attrs = [], result_locs = [], sym_name = "Foo"}> ({
  ^bb0(%arg1: !seq.clock, %arg2: i1):
    %0 = "hw.constant"() <{value = -1 : i2}> : () -> i2
    %1 = "hw.constant"() <{value = 0 : i2}> : () -> i2
    %2 = "hw.constant"() <{value = true}> : () -> i1
    %3 = "hw.constant"() <{value = false}> : () -> i1
    "sv.ifdef"() <{cond = #sv<macro.ident @layer_Foo$Verif>}> ({
      %4 = "seq.firreg"(%10, %arg1, %arg2, %1) <{name = "go_c_value"}> {firrtl.random_init_start = 0 : ui64} : (i2, !seq.clock, i1, i2) -> i2
      %5 = "hw.wire"(%6) <{name = "go"}> : (i1) -> i1
      %6 = "comb.icmp"(%4, %0) <{predicate = 0 : i64, twoState}> {sv.namehint = "go_wrap_wrap"} : (i2, i2) -> i1
      %7 = "comb.concat"(%3, %4) : (i1, i2) -> i3
      %8 = "comb.concat"(%1, %2) : (i2, i1) -> i3
      %9 = "comb.add"(%7, %8) <{twoState}> {sv.namehint = "_go_wrap_value_T"} : (i3, i3) -> i3
      %10 = "comb.extract"(%9) <{lowBit = 0 : i32}> {sv.namehint = "_go_wrap_value_T_1"} : (i3) -> i2
      "hw.instance"(%11) <{argNames = ["x"], instanceName = "e", moduleName = @E, parameters = [], resultNames = []}> : (i1) -> ()
      %11 = "comb.mux"(%5, %5, %3) <{twoState}> : (i1, i1, i1) -> i1
    }, {
    }) : () -> ()
    "hw.output"() : () -> ()
  }) : () -> ()
  "om.class"() <{fieldNames = [], fieldTypes = {}, formalParamNames = ["basepath"], sym_name = "Foo_Class"}> ({
  ^bb0(%arg0: !om.basepath):
    "om.class.fields"() <{field_locs = []}> : () -> ()
  }) : () -> ()
}) : () -> ()

[dtzSiFive]

Why the dominance check, anyway? Aren't these graph regions? 🤔

[rwy7]

I was looking at this with @dtzSiFive, here are a few notes. The sv.ifdef region is not actually a graph region (it should be, there is a dedicated procedural ifdef op with SSA regions), which causes the initial issue. We can fix this easily. The second problem is that the seq-to-sv pass (in the fir-reg-lowering code) emits register initialization code at the end of a module, and because the register is buried under an ifdef, it doesn't dominate its own inititalization.

Reproducer:

module {
  sv.macro.decl @layer_Foo$A
  hw.module @Foo(in %clock : !seq.clock, in %reset : i1, in %value : i1) {
    %c = hw.constant 0 : i1
    sv.ifdef @layer_Foo$A {
      %reg = seq.firreg %value clock %clock reset sync %reset, %c : i1
    }
    hw.output
  }
}

Sample Run:

./bin/circt-opt --lower-seq-to-sv ../scratch.mlir --mlir-print-ir-after-all
../scratch.mlir:6:14: error: operand #0 does not dominate this use
      %reg = seq.firreg %value clock %clock reset sync %reset, %c : i1
             ^
../scratch.mlir:6:14: note: see current operation: "sv.bpassign"(%11, %7) : (!hw.inout<i1>, i1) -> ()
../scratch.mlir:6:14: note: operand defined here (op is neither in a parent nor in a child region)

Requires this patch:

 void FirRegLowering::lower() {
   // Find all registers to lower in the module.
-  auto regs = module.getOps<seq::FirRegOp>();
-  if (regs.empty())
-    return;
+  SmallVector<seq::FirRegOp> regs;
+  module->walk([&](seq::FirRegOp reg) { regs.push_back(reg); });

Broken IR:

// -----// IR Dump After LowerSeqToSV Failed (lower-seq-to-sv) //----- //
"builtin.module"() ({
  "sv.macro.decl"() <{sym_name = "SYNTHESIS"}> : () -> ()
  "sv.macro.decl"() <{sym_name = "VERILATOR"}> : () -> ()
  "emit.fragment"() <{sym_name = "RANDOM_INIT_FRAGMENT"}> ({
    "sv.verbatim"() <{format_string = "// Standard header to adapt well known macros for register randomization.", symbols = []}> : () -> ()
    "sv.verbatim"() <{format_string = "\0A// RANDOM may be set to an expression that produces a 32-bit random unsigned value.", symbols = []}> : () -> ()
    "sv.ifdef"() <{cond = #sv<macro.ident @RANDOM>}> ({
    ^bb0:
    }, {
      "sv.macro.def"() <{format_string = "$random", macroName = @RANDOM, symbols = []}> : () -> ()
    }) : () -> ()
    "sv.verbatim"() <{format_string = "\0A// Users can define INIT_RANDOM as general code that gets injected into the\0A// initializer block for modules with registers.", symbols = []}> : () -> ()
    "sv.ifdef"() <{cond = #sv<macro.ident @INIT_RANDOM>}> ({
    ^bb0:
    }, {
      "sv.macro.def"() <{format_string = "", macroName = @INIT_RANDOM, symbols = []}> : () -> ()
    }) : () -> ()
    "sv.verbatim"() <{format_string = "\0A// If using random initialization, you can also define RANDOMIZE_DELAY to\0A// customize the delay used, otherwise 0.002 is used.", symbols = []}> : () -> ()
    "sv.ifdef"() <{cond = #sv<macro.ident @RANDOMIZE_DELAY>}> ({
    ^bb0:
    }, {
      "sv.macro.def"() <{format_string = "0.002", macroName = @RANDOMIZE_DELAY, symbols = []}> : () -> ()
    }) : () -> ()
    "sv.verbatim"() <{format_string = "\0A// Define INIT_RANDOM_PROLOG_ for use in our modules below.", symbols = []}> : () -> ()
    "sv.ifdef"() <{cond = #sv<macro.ident @INIT_RANDOM_PROLOG_>}> ({
    ^bb0:
    }, {
      "sv.ifdef"() <{cond = #sv<macro.ident @RANDOMIZE>}> ({
        "sv.ifdef"() <{cond = #sv<macro.ident @VERILATOR>}> ({
          "sv.macro.def"() <{format_string = "`INIT_RANDOM", macroName = @INIT_RANDOM_PROLOG_, symbols = []}> : () -> ()
        }, {
          "sv.macro.def"() <{format_string = "`INIT_RANDOM #`RANDOMIZE_DELAY begin end", macroName = @INIT_RANDOM_PROLOG_, symbols = []}> : () -> ()
        }) : () -> ()
      }, {
        "sv.macro.def"() <{format_string = "", macroName = @INIT_RANDOM_PROLOG_, symbols = []}> : () -> ()
      }) : () -> ()
    }) : () -> ()
  }) : () -> ()
  "emit.fragment"() <{sym_name = "RANDOM_INIT_REG_FRAGMENT"}> ({
    "sv.verbatim"() <{format_string = "\0A// Include register initializers in init blocks unless synthesis is set", symbols = []}> : () -> ()
    "sv.ifdef"() <{cond = #sv<macro.ident @RANDOMIZE>}> ({
    ^bb0:
    }, {
      "sv.ifdef"() <{cond = #sv<macro.ident @RANDOMIZE_REG_INIT>}> ({
        "sv.macro.def"() <{format_string = "", macroName = @RANDOMIZE, symbols = []}> : () -> ()
      }, {
      }) : () -> ()
    }) : () -> ()
    "sv.ifdef"() <{cond = #sv<macro.ident @SYNTHESIS>}> ({
    ^bb0:
    }, {
      "sv.ifdef"() <{cond = #sv<macro.ident @ENABLE_INITIAL_REG_>}> ({
      ^bb0:
      }, {
        "sv.macro.def"() <{format_string = "", macroName = @ENABLE_INITIAL_REG_, symbols = []}> : () -> ()
      }) : () -> ()
    }) : () -> ()
    "sv.verbatim"() <{format_string = "", symbols = []}> : () -> ()
  }) : () -> ()
  "sv.macro.decl"() <{sym_name = "ENABLE_INITIAL_REG_"}> : () -> ()
  "sv.macro.decl"() <{sym_name = "ENABLE_INITIAL_MEM_"}> : () -> ()
  "sv.macro.decl"() <{sym_name = "FIRRTL_BEFORE_INITIAL"}> : () -> ()
  "sv.macro.decl"() <{sym_name = "FIRRTL_AFTER_INITIAL"}> : () -> ()
  "sv.macro.decl"() <{sym_name = "RANDOMIZE_REG_INIT"}> : () -> ()
  "sv.macro.decl"() <{sym_name = "RANDOMIZE"}> : () -> ()
  "sv.macro.decl"() <{sym_name = "RANDOMIZE_DELAY"}> : () -> ()
  "sv.macro.decl"() <{sym_name = "RANDOM"}> : () -> ()
  "sv.macro.decl"() <{sym_name = "INIT_RANDOM"}> : () -> ()
  "sv.macro.decl"() <{sym_name = "INIT_RANDOM_PROLOG_"}> : () -> ()
  "sv.macro.decl"() <{sym_name = "layer_Foo$A"}> : () -> ()
  "hw.module"() <{module_type = !hw.modty<input clock : i1, input reset : i1, input value : i1>, parameters = [], sym_name = "Foo"}> ({
  ^bb0(%arg0: i1, %arg1: i1, %arg2: i1):
    %0 = "hw.constant"() <{value = 0 : i0}> : () -> i0
    %1 = "hw.constant"() <{value = true}> : () -> i1
    %2 = "hw.constant"() <{value = false}> : () -> i1
    %3 = "hw.constant"() <{value = false}> : () -> i1
    "sv.ifdef"() <{cond = #sv<macro.ident @layer_Foo$A>}> ({
      %11 = "sv.reg"() <{name = "reg"}> : () -> !hw.inout<i1>
      %12 = "sv.read_inout"(%11) : (!hw.inout<i1>) -> i1
      "sv.always"(%arg0) <{events = [0 : i32]}> ({
        "sv.if"(%arg1) ({
          "sv.passign"(%11, %3) : (!hw.inout<i1>, i1) -> ()
        }, {
          "sv.passign"(%11, %arg2) : (!hw.inout<i1>, i1) -> ()
        }) : (i1) -> ()
      }) : (i1) -> ()
    }, {
    }) : () -> ()
    "sv.ifdef"() <{cond = #sv<macro.ident @ENABLE_INITIAL_REG_>}> ({
      "sv.ordered"() ({
        "sv.ifdef"() <{cond = #sv<macro.ident @FIRRTL_BEFORE_INITIAL>}> ({
          "sv.verbatim"() <{format_string = "`FIRRTL_BEFORE_INITIAL", symbols = []}> : () -> ()
        }, {
        }) : () -> ()
        "sv.initial"() ({
          "sv.ifdef.procedural"() <{cond = #sv<macro.ident @INIT_RANDOM_PROLOG_>}> ({
            "sv.verbatim"() <{format_string = "`INIT_RANDOM_PROLOG_", symbols = []}> : () -> ()
          }, {
          }) : () -> ()
          "sv.ifdef.procedural"() <{cond = #sv<macro.ident @RANDOMIZE_REG_INIT>}> ({
            %4 = "sv.logic"() <{name = "_RANDOM"}> : () -> !hw.inout<uarray<1xi32>>
            "sv.for"(%2, %1, %1) <{inductionVarName = "i"}> ({
            ^bb0(%arg3: i1):
              %8 = "sv.macro.ref.expr.se"() <{macroName = @RANDOM}> : () -> i32
              %9 = "comb.extract"(%arg3) <{lowBit = 0 : i32}> : (i1) -> i0
              %10 = "sv.array_index_inout"(%4, %9) : (!hw.inout<uarray<1xi32>>, i0) -> !hw.inout<i32>
              "sv.bpassign"(%10, %8) : (!hw.inout<i32>, i32) -> ()
            }) : (i1, i1, i1) -> ()
            %5 = "sv.array_index_inout"(%4, %0) : (!hw.inout<uarray<1xi32>>, i0) -> !hw.inout<i32>
            %6 = "sv.read_inout"(%5) : (!hw.inout<i32>) -> i32
            %7 = "comb.extract"(%6) <{lowBit = 0 : i32}> : (i32) -> i1
            "sv.bpassign"(%11, %7) : (!hw.inout<i1>, i1) -> ()
          }, {
          }) : () -> ()
        }) : () -> ()
        "sv.ifdef"() <{cond = #sv<macro.ident @FIRRTL_AFTER_INITIAL>}> ({
          "sv.verbatim"() <{format_string = "`FIRRTL_AFTER_INITIAL", symbols = []}> : () -> ()
        }, {
        }) : () -> ()
      }) : () -> ()
    }, {
    }) : () -> ()
    "hw.output"() : () -> ()
  }) {emit.fragments = [@RANDOM_INIT_REG_FRAGMENT, @RANDOM_INIT_FRAGMENT]} : () -> ()
}) : () -> ()

[rwy7]

Here is a fix for the sv-ifdef's region-kind being wrong: #8468