Fix bit-select sensitivity lists leaking LLHD ops (Fixes #9469)

[5iri]

Summary of the fixes made

• MooreToCore.cpp - Made llhd.wait observe the i1 extracted bit instead of the i8 signal:
  - Added logic to trace extract ops to their underlying signal
  - Replace multi-bit observed values with their i1 trigger counterparts
  • Mem2Reg.cpp - Extended captureAcrossWait to handle comb.extract results:
    • Previously only captured ProbeOp results across wait
    • Now also captures extract ops of probes that are live across wait
    • This allows the i1 extract to be passed as a block argument instead of the i8 probe
  • Deseq.cpp - Two fixes:
    • Added logic to recognize equivalent extracts from the same signal at the same trigger
    • Added logic in implementRegister to create an extract outside the process when the clock trigger is an extract inside

The result: @(posedge data[0]) now correctly produces seq.firreg with the
extracted bit as the clock and solves #9469

[5iri]

@fabianschuiki done!

[5iri]

@fabianschuiki can you approve the workflow to confirm once?

[fabianschuiki]

Hey @5iri, thanks a lot for taking a stab at fixing this! I see you've had to tweak things in a few different places, spread across Deseq, Mem2Reg, and MooreToCore. Are all of these necessary? The changes feel to very specifically allow for a[0] array indexing. What happens if there are other indices? Larger slices? What if the user is providing a struct field like a.clk instead of an array index? These all feel like they would have the same issue. Could the underlying problem simply be Mem2Reg not handling the array index access properly? Or that Deseq is not tracing clocks at the correct granularity?

[5iri]

ohh yeah I didn't think about that!

I was kind of fixated on fixing it for that particular example that I was going through each layer to fix them and not the other way around. I will work on a[n] array indexing and would also need to work on multi-bit.

I’m going to broaden the trigger handling to cover any comb.extract (array/struct projections too), hoist that bit out of the process, and add a regression with @(posedge clkin_data[n]).

[fabianschuiki]

That sounds great! My suspicion is that you probably don't need to have code handling all this specific cases in a lot of different places. This is probably down to Mem2Reg not handling field projections well enough, or Deseq having to be tought about projections.

[5iri]

@fabianschuiki I think I found the issue!

in Mem2Reg, inside Promoter::captureAcrossWait, we currently handle probes and comb.extract of probes. We need to generalize this capture to any projection rooted at a probe (at least comb.extract of probe inputs) and ensure the captured value replaces the wait operand.

I think this can be done by a simple general fix which is to make Mem2Reg capture any projection whose operand is a llhd.probe, not just comb.extract.

[5iri]

it still seems to fail :(

I am now thinking there is a problem in the way the bit-select is observed.

since I see %7 = comb.extract %clkin_data from 2 : (i64) -> i1 but there is still the llhd.wait op slipping out since the observeValues helper falls back to all inputs and llhd-deseq skips it as it requires i1 but the input is i64.

from what I have observed, Deseq aborts because the clock pattern matcher (matchDriveClock) only accepts two exact DNFTerm encodings and the current table comes out as x & !a1.

trigger isn’t one of the hard-coded patterns. To make this work generally for any single-bit bit‑select clock, we need to relax matchDriveClock to accept arbitrary single‑trigger tables (maybe derive enable if I can, otherwise treat it as a plain clock) instead of insisting on those exact masks.

EDIT:
ig changing the masks is not good as well, as those masks are for defining posedge and negedge iirc

[fabianschuiki]

The fact that Deseq sees a x & !a1 term is a nice observation! Instead of adjusting the clock edge matching logic, I would try and see if we can find a better way to do lightweight alias analysis before we build these x & !a1 terms. It looks like a1 would be the previous clock value, and then instead of having the current clock value be a2, it gets absorbed into a "random other stuff" term x. That probably happens because there is a bit extract in the way there.

Deseq currently assumes that there are no aliases and that a clock has been reduced to a single SSA value. In theory, if Mem2Reg and Common Subexpression Elimination do their job properly, the clock value seen by Deseq should still be a single SSA value, just coming from a single comb.extract instead of a clock port directly. Maybe this is not the case, and Deseq gets an input IR that contains multiple distinct comb.extract for the clocks it analyzes? Do you have a snippet of the IR just before Deseq that you are working on? It might make sense to figure out the individual issues in Mem2Reg and Deseq, and create small separate PRs to fix them.

[5iri]

you are right!

I do see multiple comb.extract's in the IR before Deseq

I'll share the snippet below

module {
  hw.module @top_arc(in %clkin_data : i64, in %in_data : i192, out out_data : i192) {
    %true = hw.constant true
    %true_0 = hw.constant true
    %0 = llhd.constant_time <0ns, 0d, 1e>
    %false = hw.constant false
    %c0_i192 = hw.constant 0 : i192
    %c0_i64 = hw.constant 0 : i64
    %c0_i6 = hw.constant 0 : i6
    %_00_ = llhd.sig %c0_i6 : i6
    %clkin_data_1 = llhd.sig name "clkin_data" %c0_i64 : i64
    %1 = llhd.prb %clkin_data_1 : i64
    %in_data_2 = llhd.sig name "in_data" %c0_i192 : i192
    %2 = llhd.prb %in_data_2 : i192
    %out_data = llhd.sig %c0_i192 : i192
    %3 = comb.extract %1 from 32 : (i64) -> i1
    %rst = llhd.sig %false : i1
    %4 = llhd.prb %rst : i1
    llhd.drv %rst, %3 after %0 : i1
    %5 = llhd.constant_time <0ns, 1d, 0e>
    %6:2 = llhd.process -> i6, i1 {
      %c0_i6_3 = hw.constant 0 : i6
      %false_4 = hw.constant false
      cf.br ^bb1(%1, %c0_i6_3, %false_4 : i64, i6, i1)
    ^bb1(%8: i64, %9: i6, %10: i1):  // 4 preds: ^bb0, ^bb2, ^bb4, ^bb5
      %11 = comb.extract %8 from 0 : (i64) -> i1
      llhd.wait yield (%9, %10 : i6, i1), (%1 : i64), ^bb2(%11 : i1)
    ^bb2(%12: i1):  // pred: ^bb1
      %13 = comb.extract %1 from 0 : (i64) -> i1
      %14 = comb.xor bin %12, %true_0 : i1
      %15 = comb.and bin %14, %13 : i1
      %c0_i6_5 = hw.constant 0 : i6
      %false_6 = hw.constant false
      cf.cond_br %15, ^bb3, ^bb1(%1, %c0_i6_5, %false_6 : i64, i6, i1)
    ^bb3:  // pred: ^bb2
      %16 = comb.xor %4, %true_0 : i1
      cf.cond_br %16, ^bb4, ^bb5
    ^bb4:  // pred: ^bb3
      cf.br ^bb1(%1, %c0_i6, %true : i64, i6, i1)
    ^bb5:  // pred: ^bb3
      %true_7 = hw.constant true
      %17 = comb.extract %2 from 2 : (i192) -> i6
      cf.br ^bb1(%1, %17, %true_7 : i64, i6, i1)
    }
    llhd.drv %_00_, %6#0 after %5 if %6#1 : i6
    llhd.drv %clkin_data_1, %clkin_data after %0 : i64
    llhd.drv %in_data_2, %in_data after %0 : i192
    %7 = llhd.prb %out_data : i192
    hw.output %7 : i192
  }
}

[fabianschuiki]

Thanks for sharing! That %8 passed into bb1 is interesting. Both predecessors pass in %1, the 64 bit value, which effectively makes comb.extract %8 from 0 the comb.extract %1 from 0 that the posedge detection logic uses. But somehow these don't get simplified enough, or Deseq can't look through values enough to figure this out.

I'd suggest starting with a simpler, minimal input example that compares a version that works with a version that doesn't:

module Foo(input bit clk, input bit x, output bit y);
  always_ff @(posedge clk) x <= y;
endmodule

module Bar(input bit [41:0] clk, input bit x, output bit y);
  always_ff @(posedge clk[9]) x <= y;
endmodule

Running this through circt-verilog --ir-llhd, to get the IR before the LLHD Mem2Reg and Deseq passes run, gives this output:

hw.module @Foo(in %clk : i1, in %x : i1, out y : i1) {
  %0 = llhd.constant_time <0ns, 0d, 1e>
  %1 = llhd.constant_time <0ns, 1d, 0e>
  %true = hw.constant true
  %false = hw.constant false
  %clk_0 = llhd.sig name "clk" %false : i1
  %2 = llhd.prb %clk_0 : i1
  %x_1 = llhd.sig name "x" %false : i1
  %y = llhd.sig %false : i1
  llhd.process {
    cf.br ^bb1
  ^bb1:  // 3 preds: ^bb0, ^bb2, ^bb3
    %4 = llhd.prb %clk_0 : i1  // <--- single clock bit
    llhd.wait (%2 : i1), ^bb2
  ^bb2:  // pred: ^bb1
    %5 = llhd.prb %clk_0 : i1  // <--- single clock bit
    %6 = comb.xor bin %4, %true : i1
    %7 = comb.and bin %6, %5 : i1
    cf.cond_br %7, ^bb3, ^bb1
  ^bb3:  // pred: ^bb2
    %8 = llhd.prb %y : i1
    llhd.drv %x_1, %8 after %1 : i1
    cf.br ^bb1
  }
  llhd.drv %clk_0, %clk after %0 : i1
  llhd.drv %x_1, %x after %0 : i1
  %3 = llhd.prb %y : i1
  hw.output %3 : i1
}
hw.module @Bar(in %clk : i42, in %x : i1, out y : i1) {
  %0 = llhd.constant_time <0ns, 0d, 1e>
  %1 = llhd.constant_time <0ns, 1d, 0e>
  %true = hw.constant true
  %false = hw.constant false
  %c0_i42 = hw.constant 0 : i42
  %clk_0 = llhd.sig name "clk" %c0_i42 : i42
  %2 = llhd.prb %clk_0 : i42
  %x_1 = llhd.sig name "x" %false : i1
  %y = llhd.sig %false : i1
  llhd.process {
    cf.br ^bb1
  ^bb1:  // 3 preds: ^bb0, ^bb2, ^bb3
    %4 = llhd.prb %clk_0 : i42                 // <--- multiple clock bits
    %5 = comb.extract %4 from 9 : (i42) -> i1  // <--- multiple clock bits
    llhd.wait (%2 : i42), ^bb2
  ^bb2:  // pred: ^bb1
    %6 = llhd.prb %clk_0 : i42                 // <--- multiple clock bits
    %7 = comb.extract %6 from 9 : (i42) -> i1  // <--- multiple clock bits
    %8 = comb.xor bin %5, %true : i1
    %9 = comb.and bin %8, %7 : i1
    cf.cond_br %9, ^bb3, ^bb1
  ^bb3:  // pred: ^bb2
    %10 = llhd.prb %y : i1
    llhd.drv %x_1, %10 after %1 : i1
    cf.br ^bb1
  }
  llhd.drv %clk_0, %clk after %0 : i42
  llhd.drv %x_1, %x after %0 : i1
  %3 = llhd.prb %y : i1
  hw.output %3 : i1
}

This looks pretty good and as you would expect. Running this further to see how things look just after LLHD's hoist signals pass with circt-verilog --mlir-print-ir-after=llhd-hoist-signals, which moves probes and drives out of the process, gives the following:

hw.module @Foo(in %clk : i1, in %x : i1, out y : i1) {
  %0 = llhd.constant_time <0ns, 0d, 1e>
  %true = hw.constant true
  %false = hw.constant false
  %clk_0 = llhd.sig name "clk" %false : i1
  %1 = llhd.prb %clk_0 : i1
  %x_1 = llhd.sig name "x" %false : i1
  %y = llhd.sig %false : i1
  %2 = llhd.constant_time <0ns, 1d, 0e>
  %3:2 = llhd.process -> i1, i1 {
    %false_2 = hw.constant false
    %false_3 = hw.constant false
    cf.br ^bb1(%1, %false_2, %false_3 : i1, i1, i1)
  ^bb1(%5: i1, %6: i1, %7: i1):  // 3 preds: ^bb0, ^bb2, ^bb3
    llhd.wait yield (%6, %7 : i1, i1), (%1 : i1), ^bb2(%5 : i1)
  ^bb2(%8: i1):  // pred: ^bb1
    %9 = comb.xor bin %8, %true : i1
    %10 = comb.and bin %9, %1 : i1
    %false_4 = hw.constant false
    %false_5 = hw.constant false
    cf.cond_br %10, ^bb3, ^bb1(%1, %false_4, %false_5 : i1, i1, i1)
  ^bb3:  // pred: ^bb2
    %true_6 = hw.constant true
    cf.br ^bb1(%1, %4, %true_6 : i1, i1, i1)
  }
  llhd.drv %x_1, %3#0 after %2 if %3#1 : i1
  llhd.drv %clk_0, %clk after %0 : i1
  llhd.drv %x_1, %x after %0 : i1
  %4 = llhd.prb %y : i1
  hw.output %4 : i1
}
hw.module @Bar(in %clk : i42, in %x : i1, out y : i1) {
  %0 = llhd.constant_time <0ns, 0d, 1e>
  %true = hw.constant true
  %false = hw.constant false
  %c0_i42 = hw.constant 0 : i42
  %clk_0 = llhd.sig name "clk" %c0_i42 : i42
  %1 = llhd.prb %clk_0 : i42
  %x_1 = llhd.sig name "x" %false : i1
  %y = llhd.sig %false : i1
  %2 = llhd.constant_time <0ns, 1d, 0e>
  %3:2 = llhd.process -> i1, i1 {
    %5 = llhd.prb %clk_0 : i42
    %false_2 = hw.constant false
    %false_3 = hw.constant false
    cf.br ^bb1(%5, %false_2, %false_3 : i42, i1, i1)
  ^bb1(%6: i42, %7: i1, %8: i1):  // 3 preds: ^bb0, ^bb2, ^bb3
    %9 = comb.extract %6 from 9 : (i42) -> i1
    llhd.wait yield (%7, %8 : i1, i1), (%1 : i42), ^bb2
  ^bb2:  // pred: ^bb1
    %10 = llhd.prb %clk_0 : i42
    %11 = comb.extract %10 from 9 : (i42) -> i1
    %12 = comb.xor bin %9, %true : i1
    %13 = comb.and bin %12, %11 : i1
    %false_4 = hw.constant false
    %false_5 = hw.constant false
    cf.cond_br %13, ^bb3, ^bb1(%10, %false_4, %false_5 : i42, i1, i1)
  ^bb3:  // pred: ^bb2
    %true_6 = hw.constant true
    cf.br ^bb1(%10, %4, %true_6 : i42, i1, i1)
  }
  llhd.drv %x_1, %3#0 after %2 if %3#1 : i1
  llhd.drv %clk_0, %clk after %0 : i42
  llhd.drv %x_1, %x after %0 : i1
  %4 = llhd.prb %y : i1
  hw.output %4 : i1
}

The important difference here seems to be this (lots of identical stuff deleted):

// hw.module @Foo
%1 = llhd.prb %clk_0 : i1
%3:2 = llhd.process -> i1, i1 {
  cf.br ^bb1(%1, %false_2, %false_3 : i1, i1, i1)  // <--- passes global %1 to bb1 arg %5
^bb1(%5: i1, %6: i1, %7: i1):  // 3 preds: ^bb0, ^bb2, ^bb3
  llhd.wait yield (%6, %7 : i1, i1), (%1 : i1), ^bb2(%5 : i1)  // <--- old clock is a block arg
^bb2(%8: i1):  // pred: ^bb1
  // detect posedge with %8 and %1
  cf.cond_br %10, ^bb3, ^bb1(%1, %false_4, %false_5 : i1, i1, i1)  // <--- passes global %1 to bb1 arg %5
}

// hw.module @Bar
%1 = llhd.prb %clk_0 : i42
%3:2 = llhd.process -> i1, i1 {
  %5 = llhd.prb %clk_0 : i42  // <--- probe not hoisted out for some reason
  cf.br ^bb1(%5, %false_2, %false_3 : i42, i1, i1)  // <--- passes local %5 to bb1 arg %6
^bb1(%6: i42, %7: i1, %8: i1):  // 3 preds: ^bb0, ^bb2, ^bb3
  %9 = comb.extract %6 from 9 : (i42) -> i1
  llhd.wait yield (%7, %8 : i1, i1), (%1 : i42), ^bb2  // <--- old clock not a block arg
^bb2:  // pred: ^bb1
  %10 = llhd.prb %clk_0 : i42  // <--- probe not hoisted out for some reason
  %11 = comb.extract %10 from 9 : (i42) -> i1
  // detect posedge with %9 and %11
  cf.cond_br %13, ^bb3, ^bb1(%10, %false_4, %false_5 : i42, i1, i1)  // <--- passes local %10 to bb1 arg %6
}

For some reason, LLHD's HoistSignals pass does not properly hoist that llhd.prb %clk_0 out in the multi-bit case, but it does do so in the single-bit case. I think this is one of the main culprits. If it did hoist things out properly, the code would look something like this:

hw.module @Bar(in %clk : i42, in %x : i1, out y : i1) {
  %0 = llhd.constant_time <0ns, 0d, 1e>
  %true = hw.constant true
  %false = hw.constant false
  %c0_i42 = hw.constant 0 : i42
  %clk_0 = llhd.sig name "clk" %c0_i42 : i42
  %1 = llhd.prb %clk_0 : i42
  %x_1 = llhd.sig name "x" %false : i1
  %y = llhd.sig %false : i1
  %2 = llhd.constant_time <0ns, 1d, 0e>
  %3:2 = llhd.process -> i1, i1 {
    %false_2 = hw.constant false
    %false_3 = hw.constant false
    cf.br ^bb1(%1, %false_2, %false_3 : i42, i1, i1)
  ^bb1(%6: i42, %7: i1, %8: i1):  // 3 preds: ^bb0, ^bb2, ^bb3
    %9 = comb.extract %6 from 9 : (i42) -> i1
    llhd.wait yield (%7, %8 : i1, i1), (%1 : i42), ^bb2(%9 : i1)
  ^bb2(%10: i1):  // pred: ^bb1
    %11 = comb.extract %1 from 9 : (i42) -> i1
    %12 = comb.xor bin %10, %true : i1
    %13 = comb.and bin %12, %11 : i1
    %false_4 = hw.constant false
    %false_5 = hw.constant false
    cf.cond_br %13, ^bb3, ^bb1(%1, %false_4, %false_5 : i42, i1, i1)
  ^bb3:  // pred: ^bb2
    %true_6 = hw.constant true
    cf.br ^bb1(%1, %4, %true_6 : i42, i1, i1)
  }
  llhd.drv %x_1, %3#0 after %2 if %3#1 : i1
  llhd.drv %clk_0, %clk after %0 : i42
  llhd.drv %x_1, %x after %0 : i1
  %4 = llhd.prb %y : i1
  hw.output %4 : i1
}

Once this is hoisted out properly, we can go teach Deseq to look through comb.extract and other simple projection operations when it does its analysis. WDYT?

[5iri]

I hadn't checked whether the hoisting works in my current solution and it seems to be working as expected.

The current implementation that I have done doesn't have the multiple probe issue.

// -----// IR Dump After DeseqPass (llhd-deseq) //----- //
hw.module @Bar(in %clk : i42, in %x : i1, out y : i1) {
  %0 = llhd.constant_time <0ns, 0d, 1e>
  %true = hw.constant true
  %false = hw.constant false
  %c0_i42 = hw.constant 0 : i42
  %clk_0 = llhd.sig name "clk" %c0_i42 : i42
  %1 = llhd.prb %clk_0 : i42
  %x_1 = llhd.sig name "x" %false : i1
  %y = llhd.sig %false : i1
  %2 = llhd.constant_time <0ns, 1d, 0e>
  %3:2 = llhd.combinational -> i1, i1 {
    %true_3 = hw.constant true
    %false_4 = hw.constant false
    %false_5 = hw.constant false
    cf.br ^bb1(%1, %false_5, %false_4 : i42, i1, i1)
  ^bb1(%8: i42, %9: i1, %10: i1):  // pred: ^bb0
    %11 = comb.extract %1 from 9 : (i42) -> i1
    %false_6 = hw.constant false
    cf.cond_br %11, ^bb2, ^bb3(%1, %false_6, %false_4 : i42, i1, i1)
  ^bb2:  // pred: ^bb1
    cf.br ^bb3(%1, %7, %true_3 : i42, i1, i1)
  ^bb3(%12: i42, %13: i1, %14: i1):  // 2 preds: ^bb1, ^bb2
    llhd.yield %13, %14 : i1, i1
  }
  %4 = comb.extract %1 from 9 : (i42) -> i1
  %5 = seq.to_clock %4
  %x_2 = seq.firreg %3#0 clock %5 {name = "x"} : i1
  %6 = llhd.constant_time <0ns, 0d, 1e>
  llhd.drv %x_1, %x_2 after %6 : i1
  llhd.drv %clk_0, %clk after %0 : i42
  llhd.drv %x_1, %x after %0 : i1
  %7 = llhd.prb %y : i1
  hw.output %7 : i1
}

hw.module @Bar(in %clk : i42, in %x : i1, out y : i1) {
  %0 = llhd.constant_time <0ns, 0d, 1e>
  %true = hw.constant true
  %false = hw.constant false
  %c0_i42 = hw.constant 0 : i42
  %clk_0 = llhd.sig name "clk" %c0_i42 : i42
  %1 = llhd.prb %clk_0 : i42
  %x_1 = llhd.sig name "x" %false : i1
  %y = llhd.sig %false : i1
  %2 = llhd.constant_time <0ns, 1d, 0e>
  %3:2 = llhd.process -> i1, i1 {
    %false_2 = hw.constant false
    %false_3 = hw.constant false
    cf.br ^bb1(%1, %false_2, %false_3 : i42, i1, i1)
  ^bb1(%5: i42, %6: i1, %7: i1):  // 3 preds: ^bb0, ^bb2, ^bb3
    %8 = comb.extract %5 from 9 : (i42) -> i1
    llhd.wait yield (%6, %7 : i1, i1), (%1 : i42), ^bb2(%8 : i1)
  ^bb2(%9: i1):  // pred: ^bb1
    %10 = comb.extract %1 from 9 : (i42) -> i1
    %11 = comb.xor bin %9, %true : i1
    %12 = comb.and bin %11, %10 : i1
    %false_4 = hw.constant false
    %false_5 = hw.constant false
    cf.cond_br %12, ^bb3, ^bb1(%1, %false_4, %false_5 : i42, i1, i1)
  ^bb3:  // pred: ^bb2
    %true_6 = hw.constant true
    cf.br ^bb1(%1, %4, %true_6 : i42, i1, i1)
  }
  llhd.drv %x_1, %3#0 after %2 if %3#1 : i1
  llhd.drv %clk_0, %clk after %0 : i42
  llhd.drv %x_1, %x after %0 : i1
  %4 = llhd.prb %y : i1
  hw.output %4 : i1
}

I now think that this might not be the best solution to move forward, as

• Mem2Reg now treats comb.extract/array/struct slices rooted at a probe as the probe itself, captures them across waits, and promotes them into SSA so the single-bit clock is available outside the process.
• Deseq now recognizes those extracted bits as valid triggers, materializes derived clocks/enables/values outside the process, and still builds seq.firreg even when the clock bit was computed inside. It also relaxes clock matching when the “present” bit got lost, and strips redundant clock factors during specialization.

The relaxation of clock matching is my current issue and for that I am unable to find another solution.

[5iri]

I think should be able to make sure the extracted clock bit is carried as the i1 trigger via Deseq's projection handling, so the posedge term is present and no relaxation is needed, but I am unable to find a method for this.

[5iri]

Hey @fabianschuiki, I've tried to investigate further and wanted to share what I found before deciding how to proceed.

TLDR: My Mem2Reg changes seem to fully solve the problem, and I'm wondering if the HoistSignals approach is still necessary.

I compared the IR pipeline with and without my changes using your test case:

Without the changes:

• HoistSignals fails to hoist the multi-bit probe
• The extracted clock bit is not passed as a block argument across the wait
• Deseq cannot recognize the posedge pattern and fails to create seq.firreg
• The process remains in the final output

https://gist.github.com/5iri/f9c87098d5319841552b91ec8e47c96a

With my changes:

• The multi-bit probe is hoisting correctly
• The extracted bit is passed as a block argument (as expected)
• Deseq successfully recognizes the clean the new posedge pattern
• Clean seq.firreg output

https://gist.github.com/5iri/0225fa19130fdfb7115a0578c2f33004

The fix is in Mem2Reg's captureAcrossWait: it now captures comb.extract results (and potentially other projections) across waits as block arguments, treating them like the probes they're derived from.
This seems to indirectly fix the hoisting issue and gives Deseq the clean pattern it expects.

Before I go down the HoistSignals route, I wanted to check: Is there an architectural reason why fixing this in Mem2Reg is the wrong approach?
I can see arguments either way:

• Mem2Reg approach: Conceptually correct (it's about managing values across control flow), more general, one focused fix
• HoistSignals approach: Fixes the "root cause" of why multi-bit probes aren't hoisted, might be cleaner long-term

[fabianschuiki]

Oh this is super cool, thanks for your detailed investigation. I think you're onto something: the fact that the comb.extract isn't captured across the wait is very likely what blocks later hoisting of the probe. Really nice find! 😃

This makes me think that we are looking at two distinct issues:

1. Mem2Reg not capturing all values that are live across waits
2. Deseq not handling projections properly

Would you mind creating distinct PRs for these issues? I think the solution for the Mem2Reg issue can be very simple: instead of capturing probes (or projections of probes) that are live across waits, let's capture every value that's live across the wait. The pass already creates a Liveness analysis. That should have a function that gives you all live-out values of a block. Since llhd.wait is a terminator, you can ask the block it's terminating for the list of values that are live. That should allow you to capture all those values. (You might want to exclude values defined outside the parent region of the llhd.wait. Otherwise we end up capturing things like llhd.sigs and module ports.)

With Mem2Reg fixed, we can cook up a fix for the Deseq issue. I think we should just replace Value with something like a new ValueField wherever it makes sense. That struct could combine a Value with an index that points to which subfield the projection targets. The FIRRTL dialect has a concept of a FieldRef that works similarly. I'm curious though how we are going to deal with aliases. For example, how do we deal with an SSA value that targets a struct, and an SSA value that targets a field of the struct. Those are related, though. WDYT?

[5iri]

yeah this makes sense! I'll work on the PRs right away! Thank you so much for your help!