[LLHD] Fix incorrect mux generation in remove-control-flow

[vasyavasilevs]

Summary

This PR fixes a logical error in the llhd-remove-control-flow pass where the generated mux logic for merging arguments does not correctly represent the original CFG. This leads to phantom execution paths where values from one branch are incorrectly propagated into other paths.

Analysis of existing test case

The flaw is evident in the current regression test: circt/test/Dialect/LLHD/Transforms/remove-control-flow.mlir.
Input MLIR in the test:

hw.module @Basic(in %a: i42, in %b: i42, in %c: i1) {
  llhd.combinational -> i42 {
    %0 = comb.icmp eq %a, %b : i42
    %1 = comb.add %a, %b : i42
    cf.br ^bb1
  ^bb1:
    %2 = comb.sub %a, %b : i42
    cf.cond_br %0, ^bb2(%1 : i42), ^bb3(%2 : i42)
  ^bb2(%3: i42):
    %4 = comb.mul %3, %2 : i42
    %5 = comb.icmp eq %4, %b : i42
    cf.cond_br %5, ^bb3(%4 : i42), ^bb4(%3 : i42)
  ^bb3(%6: i42):
    %7 = comb.xor %1, %6 : i42
    cf.br ^bb4(%7 : i42)
  ^bb4(%8: i42):
    llhd.yield %8 : i42
  }
}

In this CFG, there are two paths leading to ^bb3:
Path A (from ^bb1): Triggered when !%0. The value passed is %2.
Path B (from ^bb2): Triggered when %0 AND %5. The value passed is %4.

The current output expected by the test:

 // CHECK-NEXT: [[TMP6A:%.+]] = hw.constant true
 // CHECK-NEXT: [[TMP6B:%.+]] = comb.xor [[TMP0]], [[TMP6A]] // This is !%0
 // CHECK-NEXT: [[TMP6:%.+]] = comb.mux [[TMP6B]], [[TMP2]], [[TMP4]] // if (!%0) %2 else %4

The mux above implements the logic: if (!%0) then %2 else %4. It completely ignores the condition %5. If %0 is true but %5 is false, the control should flow to ^bb4 directly, bypassing ^bb3. However, the generated mux logic for the value %6 (represented by [[TMP6]]) assumes that if %0 is true, the value must be %4.

In a hardware context, this creates a phantom path where the logic from ^bb3 is executed with the value of %4 even when the condition %5 is false. This was discovered during formal verification of the PicoRV32 CPU, where such phantom paths led to false-positive assertion violations.

Root Cause

The mergeArgs function in RemoveControlFlow.cpp initializes the mergedArgs with the first predecessor it encounters:

if (mergedArgs.empty()) {
    mergedArgs = args;
    return;
}

If the first predecessor encountered is a conditional branch, its values are treated as the default for all other paths, without being guarded by the actual condition that makes that path reachable.

Proposed Solution

The fix involves sorting the predecessors of a block based on dominance before generating the mux chain. By ensuring that the Dominator Block is processed first, it becomes the base of the mux tree. All other conditional paths are then layered on top as refinements. In the case of the test above, %2 (from the dominator path) will be the base, and %4 will be applied only if the specific path condition (%0 & %5) is met.

[circt-bot]

Results of circt-tests run for 0e4b7f1 compared to results for 7aa41cc: no change to test results.