[circt-verilog] Import difference of results in arcilator failure with remaining llhd constant_time

[jpienaar]

Input test case:

module bug (
    input logic wr_clk,
    input logic wr_data,
    output logic [1:0] mem
);
`ifdef CASE_1
  always_ff @(posedge (wr_clk)) begin
      mem[0] <= wr_data;
  end
  always_ff @(posedge (wr_clk)) begin
      mem[1] <= wr_data;
  end
`else
  always_ff @(posedge (wr_clk)) begin
      mem[0] <= wr_data;
      mem[1] <= wr_data;
  end
`endif
endmodule

the two results in different outputs

module {
  hw.module @bug(in %wr_clk : i1 loc(#loc2), in %wr_data : i1 loc(#loc3), out mem : i2) {
    %0 = comb.replicate %wr_data : (i1) -> i2 loc(#loc4)
    %1 = seq.to_clock %wr_clk loc(#loc5)
    %mem = seq.firreg %0 clock %1 : i2 loc(#loc5)
    hw.output %mem : i2 loc(#loc1)
  } loc(#loc1)
} loc(#loc)

vs

module {
  hw.module @bug(in %wr_clk : i1 loc(#loc2), in %wr_data : i1 loc(#loc3), out mem : i2) {
    %0 = llhd.constant_time <0ns, 0d, 1e> loc(#loc2)
    %c0_i2 = hw.constant 0 : i2 loc(#loc4)
    %mem = llhd.sig %c0_i2 : i2 loc(#loc4)
    %1 = comb.extract %6 from 1 : (i2) -> i1 loc(#loc5)
    %2 = comb.concat %1, %wr_data : i1, i1 loc(#loc5)
    %3 = seq.to_clock %wr_clk loc(#loc4)
    %mem_0 = seq.firreg %2 clock %3 {name = "mem"} : i2 loc(#loc4)
    llhd.drv %mem, %mem_0 after %0 : !hw.inout<i2> loc(#loc4)
    %4 = comb.extract %6 from 0 : (i2) -> i1 loc(#loc6)
    %5 = comb.concat %wr_data, %4 : i1, i1 loc(#loc6)
    %mem_1 = seq.firreg %5 clock %3 {name = "mem"} : i2 loc(#loc4)
    llhd.drv %mem, %mem_1 after %0 : !hw.inout<i2> loc(#loc4)
    %6 = llhd.prb %mem : !hw.inout<i2> loc(#loc4)
    hw.output %6 : i2 loc(#loc1)
  } loc(#loc1)
} loc(#loc)

(splitting the mem output into 2 separate outputs also result in same output with and without def).

[fabianschuiki]

Nice find! This feels like it might be interacting with the memory detection pass that circt-verilog runs after Deseq. That tries to find things that look like a memory and convert them from seq.firreg to seq.firmem.

[fabianschuiki]

This looks like an issue with the LLHD Mem2Reg pass, and the HoistSignals pass that runs after it.

// circt-verilog --mlir-print-ir-after-all
module bug(input logic clk, input logic a, output logic [1:0] z);
  always_ff @(posedge clk) z[0] <= a;
  always_ff @(posedge clk) z[1] <= a;
endmodule

The always processes produce the following LLHD processes just before Mem2Reg runs:

    llhd.process {
      cf.br ^bb1
    ^bb1:  // 3 preds: ^bb0, ^bb2, ^bb3
      %4 = llhd.prb %clk_0 : !hw.inout<i1>
      llhd.wait (%2 : i1), ^bb2
    ^bb2:  // pred: ^bb1
      %5 = llhd.prb %clk_0 : !hw.inout<i1>
      %6 = comb.xor bin %4, %true : i1
      %7 = comb.and bin %6, %5 : i1
      cf.cond_br %7, ^bb3, ^bb1
    ^bb3:  // pred: ^bb2
      // ------
      // first always_ff (z[0]):
      %8 = llhd.sig.extract %z from %false : (!hw.inout<i2>) -> !hw.inout<i1>
      // second always_ff (z[1]):
      %8 = llhd.sig.extract %z from %true : (!hw.inout<i2>) -> !hw.inout<i1>
      // ------
      %9 = llhd.prb %a_1 : !hw.inout<i1>
      llhd.drv %8, %9 after %1 : !hw.inout<i1>
      cf.br ^bb1
    }

Even though these processes operate entirely on two separate bits of z (I think the Verilog spec calls these "static prefices"), the Mem2Reg pass turns them both into (1) read entire z, (2) update specific bit, (3) write entire z:

  llhd.process {
    %3 = llhd.prb %clk_0 : !hw.inout<i1>
    %c0_i2_2 = hw.constant 0 : i2
    %false_3 = hw.constant false
    cf.br ^bb1(%3, %c0_i2_2, %false_3 : i1, i2, i1)
  ^bb1(%4: i1, %5: i2, %6: i1):  // 3 preds: ^bb0, ^bb2, ^bb3
    %7 = llhd.constant_time <0ns, 1d, 0e>
    llhd.drv %z, %5 after %7 if %6 : !hw.inout<i2>
    llhd.wait (%1 : i1), ^bb2(%4 : i1)
  ^bb2(%8: i1):  // pred: ^bb1
    %false_4 = hw.constant false
    %9 = llhd.prb %clk_0 : !hw.inout<i1>
    %10 = llhd.prb %z : !hw.inout<i2>
    %11 = llhd.prb %a_1 : !hw.inout<i1>
    %12 = comb.xor bin %8, %true : i1
    %13 = comb.and bin %12, %9 : i1
    cf.cond_br %13, ^bb3, ^bb1(%9, %10, %false_4 : i1, i2, i1)
  ^bb3:  // pred: ^bb2
    %true_5 = hw.constant true
    // ------
    // first always_ff (z[0]):
    %14 = comb.extract %10 from 1 : (i2) -> i1
    %15 = comb.concat %14, %11 : i1, i1
    // second always_ff (z[1]):
    %14 = comb.extract %10 from 0 : (i2) -> i1
    %15 = comb.concat %11, %14 : i1, i1
    // ------
    cf.br ^bb1(%9, %15, %true_5 : i1, i2, i1)
  }

As a result, both processes end up driving the entire z signal. This will then run through Deseq etc. as you'd expect, with two separate resulting seq.firregs that are 2 bits wide, but each only having one of the z bits really updated.

Maybe we should split signals apart if they end up being driven by separate processes. We should probably also make the Mem2Reg pass smarter such that it doesn't track the full z signal in this case, but realizes that the process only ever touches z[0] and therefore targets that specifically. This would match Verilog's notion of static prefices.

[SimonEbner]

I use simpler a workaround to merge the moore.procedures if their wait_event regions are identical for now. It is not fully generic and only supports procedures with single blocks but was useful to unblock us.
I've opened #9101 in case it's useful as a temporary workaround upstream too