[crict-verilog][Mem2Reg] Lowering for `llhd.combinational` with peculiar CFG
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 2.2k
- Forks
- 524
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 46
Description
While examining the `circt-verilog` output for Ibex I've noticed the following case (minimal reproducer)
```systemverilog
module moduleName (
);
localparam int unsigned MHPMCOUNTER_BASE = 3;
logic [31:0] mhpmevent [32];
always_comb begin : gen_mhpmevent
for (int i = 0; i < 32; i++)
begin : gen_mhpmevent_active
mhpmevent[i] = '0; // <- remove this to make llhd block disapper
if (i >= MHPMCOUNTER_BASE) begin
mhpmevent[i][i - MHPMCOUNTER_BASE] = 1'b1;
end
end
end
endmodule
```
Which generates an `llhd.combinational` block that cannot be lowered further by the pipeline:
Expand to see full output
```llvm
module {
hw.module @moduleName() {
%c14_i5 = hw.constant 14 : i5
%c13_i5 = hw.constant 13 : i5
%c15_i5 = hw.constant 15 : i5
%c12_i5 = hw.constant 12 : i5
%c-16_i5 = hw.constant -16 : i5
%c11_i5 = hw.constant 11 : i5
%c-15_i5 = hw.constant -15 : i5
%c10_i5 = hw.constant 10 : i5
%c-14_i5 = hw.constant -14 : i5
%c9_i5 = hw.constant 9 : i5
%c-13_i5 = hw.constant -13 : i5
%c8_i5 = hw.constant 8 : i5
%c-12_i5 = hw.constant -12 : i5
%c7_i5 = hw.constant 7 : i5
%c-11_i5 = hw.constant -11 : i5
%c6_i5 = hw.constant 6 : i5
%c-10_i5 = hw.constant -10 : i5
%c5_i5 = hw.constant 5 : i5
%c-9_i5 = hw.constant -9 : i5
%c4_i5 = hw.constant 4 : i5
%c-8_i5 = hw.constant -8 : i5
%c3_i5 = hw.constant 3 : i5
%c-7_i5 = hw.constant -7 : i5
%c2_i5 = hw.constant 2 : i5
%c-6_i5 = hw.constant -6 : i5
%c1_i5 = hw.constant 1 : i5
%c-5_i5 = hw.constant -5 : i5
%c0_i5 = hw.constant 0 : i5
%c-4_i5 = hw.constant -4 : i5
%c-3_i5 = hw.constant -3 : i5
%c-2_i5 = hw.constant -2 : i5
%c-1_i5 = hw.constant -1 : i5
%0 = llhd.constant_time <0ns, 0d, 1e>
%true = hw.constant true
%c0_i32 = hw.constant 0 : i32
%1 = hw.aggregate_constant [0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32, 0 : i32] : !hw.array<32xi32>
%mhpmevent = llhd.sig %1 : !hw.array<32xi32>
llhd.combinational {
%2 = llhd.sig.array_get %mhpmevent[%c-1_i5] : >
llhd.drv %2, %c0_i32 after %0 : i32
%3 = llhd.sig.array_get %mhpmevent[%c-2_i5] : >
llhd.drv %3, %c0_i32 after %0 : i32
%4 = llhd.sig.array_get %mhpmevent[%c-3_i5] : >
llhd.drv %4, %c0_i32 after %0 : i32
%5 = llhd.sig.array_get %mhpmevent[%c-4_i5] : >
llhd.drv %5, %c0_i32 after %0 : i32
%6 = llhd.sig.extract %5 from %c0_i5 : ->
llhd.drv %6, %true after %0 : i1
%7 = llhd.sig.array_get %mhpmevent[%c-5_i5] : >
llhd.drv %7, %c0_i32 after %0 : i32
%8 = llhd.sig.extract %7 from %c1_i5 : ->
llhd.drv %8, %true after %0 : i1
%9 = llhd.sig.array_get %mhpmevent[%c-6_i5] : >
llhd.drv %9, %c0_i32 after %0 : i32
%10 = llhd.sig.extract %9 from %c2_i5 : ->
llhd.drv %10, %true after %0 : i1
%11 = llhd.sig.array_get %mhpmevent[%c-7_i5] : >
llhd.drv %11, %c0_i32 after %0 : i32
%12 = llhd.sig.extract %11 from %c3_i5 : ->
llhd.drv %12, %true after %0 : i1
%13 = llhd.sig.array_get %mhpmevent[%c-8_i5] : >
llhd.drv %13, %c0_i32 after %0 : i32
%14 = llhd.sig.extract %13 from %c4_i5 : ->
llhd.drv %14, %true after %0 : i1
%15 = llhd.sig.array_get %mhpmevent[%c-9_i5] : >
llhd.drv %15, %c0_i32 after %0 : i32
%16 = llhd.sig.extract %15 from %c5_i5 : ->
llhd.drv %16, %true after %0 : i1
%17 = llhd.sig.array_get %mhpmevent[%c-10_i5] : >
llhd.drv %17, %c0_i32 after %0 : i32
%18 = llhd.sig.extract %17 from %c6_i5 : ->
llhd.drv %18, %true after %0 : i1
%19 = llhd.sig.array_get %mhpmevent[%c-11_i5] : >
llhd.drv %19, %c0_i32 after %0 : i32
%20 = llhd.sig.extract %19 from %c7_i5 : ->
llhd.drv %20, %true after %0 : i1
%21 = llhd.sig.array_get %mhpmevent[%c-12_i5] : >
llhd.drv %21, %c0_i32 after %0 : i32
%22 = llhd.sig.extract %21 from %c8_i5 : ->
llhd.drv %22, %true after %0 : i1
%23 = llhd.sig.array_get %mhpmevent[%c-13_i5] : >
llhd.drv %23, %c0_i32 after %0 : i32
%24 = llhd.sig.extract %23 from %c9_i5 : ->
llhd.drv %24, %true after %0 : i1
%25 = llhd.sig.array_get %mhpmevent[%c-14_i5] : >
llhd.drv %25, %c0_i32 after %0 : i32
%26 = llhd.sig.extract %25 from %c10_i5 : ->
llhd.drv %26, %true after %0 : i1
%27 = llhd.sig.array_get %mhpmevent[%c-15_i5] : >
llhd.drv %27, %c0_i32 after %0 : i32
%28 = llhd.sig.extract %27 from %c11_i5 : ->
llhd.drv %28, %true after %0 : i1
%29 = llhd.sig.array_get %mhpmevent[%c-16_i5] : >
llhd.drv %29, %c0_i32 after %0 : i32
%30 = llhd.sig.extract %29 from %c12_i5 : ->
llhd.drv %30, %true after %0 : i1
%31 = llhd.sig.array_get %mhpmevent[%c15_i5] : >
llhd.drv %31, %c0_i32 after %0 : i32
%32 = llhd.sig.extract %31 from %c13_i5 : ->
llhd.drv %32, %true after %0 : i1
%33 = llhd.sig.array_get %mhpmevent[%c14_i5] : >
llhd.drv %33, %c0_i32 after %0 : i32
%34 = llhd.sig.extract %33 from %c14_i5 : ->
llhd.drv %34, %true after %0 : i1
%35 = llhd.sig.array_get %mhpmevent[%c13_i5] : >
llhd.drv %35, %c0_i32 after %0 : i32
%36 = llhd.sig.extract %35 from %c15_i5 : ->
llhd.drv %36, %true after %0 : i1
%37 = llhd.sig.array_get %mhpmevent[%c12_i5] : >
llhd.drv %37, %c0_i32 after %0 : i32
%38 = llhd.sig.extract %37 from %c-16_i5 : ->
llhd.drv %38, %true after %0 : i1
%39 = llhd.sig.array_get %mhpmevent[%c11_i5] : >
llhd.drv %39, %c0_i32 after %0 : i32
%40 = llhd.sig.extract %39 from %c-15_i5 : ->
llhd.drv %40, %true after %0 : i1
%41 = llhd.sig.array_get %mhpmevent[%c10_i5] : >
llhd.drv %41, %c0_i32 after %0 : i32
%42 = llhd.sig.extract %41 from %c-14_i5 : ->
llhd.drv %42, %true after %0 : i1
%43 = llhd.sig.array_get %mhpmevent[%c9_i5] : >
llhd.drv %43, %c0_i32 after %0 : i32
%44 = llhd.sig.extract %43 from %c-13_i5 : ->
llhd.drv %44, %true after %0 : i1
%45 = llhd.sig.array_get %mhpmevent[%c8_i5] : >
llhd.drv %45, %c0_i32 after %0 : i32
%46 = llhd.sig.extract %45 from %c-12_i5 : ->
llhd.drv %46, %true after %0 : i1
%47 = llhd.sig.array_get %mhpmevent[%c7_i5] : >
llhd.drv %47, %c0_i32 after %0 : i32
%48 = llhd.sig.extract %47 from %c-11_i5 : ->
llhd.drv %48, %true after %0 : i1
%49 = llhd.sig.array_get %mhpmevent[%c6_i5] : >
llhd.drv %49, %c0_i32 after %0 : i32
%50 = llhd.sig.extract %49 from %c-10_i5 : ->
llhd.drv %50, %true after %0 : i1
%51 = llhd.sig.array_get %mhpmevent[%c5_i5] : >
llhd.drv %51, %c0_i32 after %0 : i32
%52 = llhd.sig.extract %51 from %c-9_i5 : ->
llhd.drv %52, %true after %0 : i1
%53 = llhd.sig.array_get %mhpmevent[%c4_i5] : >
llhd.drv %53, %c0_i32 after %0 : i32
%54 = llhd.sig.extract %53 from %c-8_i5 : ->
llhd.drv %54, %true after %0 : i1
%55 = llhd.sig.array_get %mhpmevent[%c3_i5] : >
llhd.drv %55, %c0_i32 after %0 : i32
%56 = llhd.sig.extract %55 from %c-7_i5 : ->
llhd.drv %56, %true after %0 : i1
%57 = llhd.sig.array_get %mhpmevent[%c2_i5] : >
llhd.drv %57, %c0_i32 after %0 : i32
%58 = llhd.sig.extract %57 from %c-6_i5 : ->
llhd.drv %58, %true after %0 : i1
%59 = llhd.sig.array_get %mhpmevent[%c1_i5] : >
llhd.drv %59, %c0_i32 after %0 : i32
%60 = llhd.sig.extract %59 from %c-5_i5 : ->
llhd.drv %60, %true after %0 : i1
%61 = llhd.sig.array_get %mhpmevent[%c0_i5] : >
llhd.drv %61, %c0_i32 after %0 : i32
%62 = llhd.sig.extract %61 from %c-4_i5 : ->
llhd.drv %62, %true after %0 : i1
llhd.yield
}
hw.output
}
}
```
From my triaging it looks like `Mem2Reg` fails to promote because of the control-flow structure here. By adding another round of `Mem2Reg` _after_ `RemoveControlFlow` in the pipeline this seems to be solved:
```llvm
module {
hw.module @moduleName() {
hw.output
}
}
```
I'm not sure if the "real" solution would be to make Mem2Reg more permissive or just moving/duplicating Mem2Reg makes sense, so leaving this up to debate.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the minimal SystemVerilog reproducer and trace the Mem2Reg and RemoveControlFlow pipeline stages that produce the unlowered llhd.combinational block. Compare the pipeline behavior with and without an additional Mem2Reg pass after RemoveControlFlow. Done means establishing the appropriate pipeline or Mem2Reg change and confirming that the reproducer lowers successfully without the residual block.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100