llvm / llvm/circt

[ExportVerilog] always_comb ordering issue

Open
#4,532 4 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
2.2k
Forks
524
Avg merge
3d 2h
Merged PRs (30d)
46

Description

The following MLIR (bear with me there's potentially a lot of un-important parts of this IR):

```mlir
module attributes {circt.loweringOptions = "locationInfoStyle=none"} {
hw.module @Main(%IA: i2, %IB: i2, %IS: i1) -> (O: i2) {
%0 = hw.constant 0 : i2
%1 = hw.constant 1 : i2
%2 = hw.constant 0 : i1
%3 = comb.extract %IA from 0 : (i2) -> i1
%4 = comb.extract %IA from 1 : (i2) -> i1
%5 = comb.extract %IB from 0 : (i2) -> i1
%6 = hw.constant 1 : i1
%7 = comb.extract %IB from 1 : (i2) -> i1
%14 = sv.reg : !hw.inout
%8 = sv.read_inout %14 : !hw.inout
%15 = sv.reg : !hw.inout
%9 = sv.read_inout %15 : !hw.inout
%16 = sv.reg : !hw.inout
%10 = sv.read_inout %16 : !hw.inout
%17 = sv.reg : !hw.inout
%11 = sv.read_inout %17 : !hw.inout
%18 = sv.reg : !hw.inout
%12 = sv.read_inout %18 : !hw.inout
%19 = sv.reg : !hw.inout
%13 = sv.read_inout %19 : !hw.inout
sv.alwayscomb {
sv.if %IS {
sv.bpassign %16, %2 : i1
sv.bpassign %17, %2 : i1
sv.bpassign %18, %5 : i1
sv.bpassign %19, %7 : i1
} else {
sv.bpassign %16, %3 : i1
sv.bpassign %17, %4 : i1
sv.bpassign %18, %6 : i1
sv.bpassign %19, %2 : i1
}
}
%20 = comb.concat %11, %11, %10 : i1, i1, i1
%21 = comb.concat %13, %13, %12 : i1, i1, i1
%22 = comb.add %20, %21 : i3
%23 = comb.extract %22 from 1 : (i3) -> i1
%24 = comb.extract %22 from 2 : (i3) -> i1
%25 = comb.concat %24, %23 : i1, i1
%26 = hw.constant 1 : i2
%27 = comb.icmp eq %25, %26 : i2
%28 = hw.constant 1 : i2
%29 = comb.concat %24, %23 : i1, i1
%30 = hw.constant -2 : i2
%31 = comb.icmp eq %29, %30 : i2
%32 = hw.constant 2 : i2
%33 = hw.constant 1 : i1
%34 = hw.constant 0 : i1
%35 = comb.extract %22 from 0 : (i3) -> i1
%39 = sv.reg : !hw.inout
%36 = sv.read_inout %39 : !hw.inout
%40 = sv.reg : !hw.inout
%37 = sv.read_inout %40 : !hw.inout
%41 = sv.reg : !hw.inout
%38 = sv.read_inout %41 : !hw.inout
sv.alwayscomb {
sv.if %27 {
sv.bpassign %40, %33 : i1
sv.bpassign %41, %34 : i1
} else {
sv.if %31 {
sv.bpassign %40, %34 : i1
sv.bpassign %41, %33 : i1
} else {
sv.bpassign %40, %35 : i1
sv.bpassign %41, %23 : i1
}
}
}
%42 = comb.concat %38, %37 : i1, i1
hw.output %42 : i2
}
}
```

produces the following Verilog:

```verilog
// Generated by CIRCT circtorg-0.0.0-1018-g3a39b339f
module Main(
input [1:0] IA,
IB,
input IS,
output [1:0] O);

reg _GEN;
reg _GEN_0;
reg _GEN_1;
reg _GEN_2;
reg _GEN_3;
reg _GEN_4;
always_comb begin
automatic logic [2:0] _GEN_5;
_GEN_5 = {{2{_GEN_0}}, _GEN} + {{2{_GEN_2}}, _GEN_1};
if (IS) begin
_GEN = 1'h0;
_GEN_0 = 1'h0;
_GEN_1 = IB[0];
_GEN_2 = IB[1];
end
else begin
_GEN = IA[0];
_GEN_0 = IA[1];
_GEN_1 = 1'h1;
_GEN_2 = 1'h0;
end
if (_GEN_5[2:1] == 2'h1) begin
_GEN_3 = 1'h1;
_GEN_4 = 1'h0;
end
else if (_GEN_5[2:1] == 2'h2) begin
_GEN_3 = 1'h0;
_GEN_4 = 1'h1;
end
else begin
_GEN_3 = _GEN_5[0];
_GEN_4 = _GEN_5[1];
end
end // always_comb
assign O = {_GEN_4, _GEN_3};
endmodule
```

The issue here is that the add (`%22 = comb.add %20, %21 : i3`) which is outside of the first `always_comb` gets pushed into the *beginning* of the `always_comb` (`_GEN_5 = ... + ...`) in the Verilog, although it depends on register assignments later in that same `always_comb`. This results in a simulation where the add uses old values of the relevant registers.

I believe that potentially using non-block assignments, *or* moving the add outside of the `always_comb` would fix this issue.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the ExportVerilog path and reproduce the issue from the supplied MLIR and generated Verilog, focusing on the two always_comb regions and the comb.add dependency. Compare the emitted statement order with the IR order and verify in simulation that the add observes the assignments from the same always_comb; done means the generated behavior no longer uses stale register values.

Written by the indexing model from the issue text.

Assessment

Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.