Repository: calyxir/calyx (language: rust)

Issue:
Refactor `std_fp_div_pipe` primitive to avoid Verilog features not supported by Icarus
It seems that portions of Calyx's standard library written in SystemVerilog is not able to be simulated by Icarus.

```sh
[nix-shell:~/Repos/calyx] 11:12:38 $ iverilog -v
Icarus Verilog version 12.0 (stable) ()

...
```

Taking a unit-test circuit from Cider and running it through `fud2`, we get the following plan, which I then execute.
```sh
[nix-shell:~/Repos/calyx] 11:25:19 $ ./target/debug/fud2 -m plan ./interp/tests/complex/unsigned-dot-product.futil --from calyx -o test.exe --through icarus -s sim.data=./interp/tests/complex/unsigned-dot-product.futil.data --keep
calyx-noverify: interp/tests/complex/unsigned-dot-product.futil -> verilog-noverify.sv
icarus: verilog-noverify.sv -> test.exe

[nix-shell:~/Repos/calyx] 11:25:31 $ ./target/debug/fud2 ./interp/tests/complex/unsigned-dot-product.futil --from calyx -o test.exe --through icarus -s sim.data=./interp/tests/complex/unsigned-dot-product.futil.data --keep
[WARN  calyx_frontend::attribute] The attribute @static is deprecated and will be ignored by the compiler.
[WARN  calyx_frontend::attribute] The attribute @static is deprecated and will be ignored by the compiler.
[WARN  calyx_frontend::attribute] The attribute @static is deprecated and will be ignored by the compiler.
[WARN  calyx_frontend::attribute] The attribute @static is deprecated and will be ignored by the compiler.
verilog-noverify.sv:149: sorry: constant selects in always_* processes are not currently supported (all bits will be included).
```

The error on line 149 is coming from:
```systemverilog
/* verilator lint_off WIDTH */
module std_fp_div_pipe #(
  parameter WIDTH = 32,
  parameter INT_WIDTH = 16,
  parameter FRAC_WIDTH = 16
) (
    input  logic             go,
    input  logic             clk,
    input  logic             reset,
    input  logic [WIDTH-1:0] left,
    input  logic [WIDTH-1:0] right,
    output logic [WIDTH-1:0] out_remainder,
    output logic [WIDTH-1:0] out_quotient,
    output logic             done
);
    localparam ITERATIONS = WIDTH + FRAC_WIDTH;

    logic [WIDTH-1:0] quotient, quotient_next;
    logic [WIDTH:0] acc, acc_next;
    logic [$clog2(ITERATIONS)-1:0] idx;
    logic start, running, finished, dividend_is_zero;

    // Omitted for brevity

    // always_comb is the problem.
    always_comb begin
      if (acc >= {1'b0, right}) begin
        acc_next = acc - right;
        {acc_next, quotient_next} = {acc_next[WIDTH-1:0], quotient, 1'b1};
      end else begin
        {acc_next, quotient_next} = {acc, quotient} << 1;
      end
    end

    // Omitted for brevity
endmodule
```

The Calyx component's output Verilog that being simulated is attached below. (It is a .txt to allow me to upload to GitHub.)
[verilog-noverify.txt](https://github.com/user-attachments/files/16532618/verilog-noverify.txt)


Discussion / hints:
Thanks! I updated the title to point to the specific issue at hand.

The first step here will be to figure out what Icarus means by "constant selects". It's something about the `if`, presumably, but I don't quite see it by just staring at the current code.
> constant selects in always_*

My immediate guess from the error being thrown is that Icarus does not like the `1'b0` or `WIDTH`. Because `always_ff` "desugars" to something like Verilog's `always` operator. I _think_ Icarus' support of SystemVerilog handles anything inside the `always_ff` block by pulling it up into the sensitivity list, and Icarus is warning you about that.

[This StackOverflow](https://stackoverflow.com/questions/71837597/error-message-sorry-constant-selects-in-always-processes-are-not-currently-s) does something similar to what is done with the bit concatenation with the constant, but using SystemVerilog's `case` instead. 

Ok, after some tinkering, I have narrowed down the problematic expression in the `always_comb`. It is the bit slicing of `acc_next`. I did some pruning just to make the module easier to read.

You can run each of these examples with the following one-line command:
```console
$ iverilog -g2012 test.v
```

```systemverilog
module std_fp_div_pipe #(
  parameter WIDTH = 32
) (
    input  logic [WIDTH-1:0] right,
    output logic             done
);

    logic [WIDTH-1:0] quotient, quotient_next;
    logic [WIDTH:0] acc, acc_next;

    // always_comb is the problem.
    always_comb begin
      if (acc >= {1'b0, right}) begin
        acc_next = acc - right;
        {acc_next, quotient_next} = {acc_next[WIDTH-1:0], quotient, 1'b1};
      end else begin
        {acc_next, quotient_next} = {acc, quotient} << 1;
      end
    end
endmodule
```
The command on this minimized version produces the error mentioned above:
```console
$ iverilog -g2012 test.v
test.v:13: sorry: constant selects in always_* processes are not currently supported (all bits will be included).
```

If I remove the bit slicing, then the error goes away.
```systemverilog
module std_fp_div_pipe #(
  parameter WIDTH = 32
) (
    input  logic [WIDTH-1:0] right,
    output logic             done
);

    logic [WIDTH-1:0] quotient, quotient_next;
    logic [WIDTH:0] acc, acc_next;

    // always_comb is the problem.
    always_comb begin
      if (acc >= {1'b0, right}) begin
        acc_next = acc - right;
        // NOTE: [WIDTH-1:0] is gone from acc_next!
        {acc_next, quotient_next} = {acc_next, quotient, 1'b1};
      end else begin
        {acc_next, quotient_next} = {acc, quotient} << 1;
      end
    end
endmodule
```

The output from this module:
```console
$ iverilog -g2012 test.v

$ echo $?
0
```
You can probably rewrite this to used `assign` instead of the `always_comb`:

```verilog
assign test  = (acc >= {1'b0, right});
assign true_branch = {acc_next, quotient, 1'b1};
assign false_branch = {acc, quotient} << 1;
assign acc_next = test ? ... : ...; // annoying slicing for `true_branch` and `false_branch`
assign quotient_next = test ? ... : ...;
```
We may not even need to go that far, depending on how Calyx uses `always_comb`. Verilog 2001 introduced syntax that mirrors SystemVerilog's `always_comb` that we should be able to use. The following worked (I did not simulate, just lint.) There are [some differences](https://www.verilogpro.com/systemverilog-always_comb-always_ff/) between `always_comb` and `always @*`; I am not sure how Calyx's surrounding infrastructure will fare with this kind of change.

```systemverilog
module std_fp_div_pipe #(
  parameter WIDTH = 32
) (
    input  logic [WIDTH-1:0] right,
    output logic             done
);

    logic [WIDTH-1:0] quotient, quotient_next;
    logic [WIDTH:0] acc, acc_next;

    // always_comb is the problem.
    always @* begin
      if (acc >= {1'b0, right}) begin
        acc_next = acc - right;
        {acc_next, quotient_next} = {acc_next[WIDTH-1:0], quotient, 1'b1};
      end else begin
        {acc_next, quotient_next} = {acc, quotient} << 1;
      end
    end
endmodule
```

```console
$ iverilog -g2012 test.v

$ echo $?
0
```
Calyx does not emit always_comb so the only uses are in the standard library.
I saw that, which means the blast radius of this kind of change will shrink. If I make this change to `std_fp_div_pipe`, is there a way to run a set of comprehensive regression tests on everything (Cider, Verilator, Icarus, ...) to make sure that changing `always_comb` to `always @*` for this module does not break flows elsewhere? I am worried about Calyx code relying on some of SystemVerilog's more advanced behavior for `always_comb` which will break under `always @*`.
Your best bet is the tests we already run in the CI. If you're suspicious of particular things failing, you can try to write more tests. 

Fix patch:
diff --git a/primitives/binary_operators.sv b/primitives/binary_operators.sv
index 1cfe5beabe..7ba413f0d8 100644
--- a/primitives/binary_operators.sv
+++ b/primitives/binary_operators.sv
@@ -146,7 +146,7 @@ module std_fp_div_pipe #(
         running <= running;
     end
 
-    always_comb begin
+    always @* begin
       if (acc >= {1'b0, right}) begin
         acc_next = acc - right;
         {acc_next, quotient_next} = {acc_next[WIDTH-1:0], quotient, 1'b1};


Test patch:
diff --git a/yxi/tests/axi/dynamic/dyn-mem-vec-add-axi-wrapped.expect b/yxi/tests/axi/dynamic/dyn-mem-vec-add-axi-wrapped.expect
index eaad492606..3b0d36f229 100644
--- a/yxi/tests/axi/dynamic/dyn-mem-vec-add-axi-wrapped.expect
+++ b/yxi/tests/axi/dynamic/dyn-mem-vec-add-axi-wrapped.expect
@@ -335,7 +335,7 @@ module std_fp_div_pipe #(
         running <= running;
     end
 
-    always_comb begin
+    always @* begin
       if (acc >= {1'b0, right}) begin
         acc_next = acc - right;
         {acc_next, quotient_next} = {acc_next[WIDTH-1:0], quotient, 1'b1};
diff --git a/yxi/tests/axi/read-compute-write/seq-mem-vec-add-axi-wrapped.expect b/yxi/tests/axi/read-compute-write/seq-mem-vec-add-axi-wrapped.expect
index bf390e5738..ac69b673cc 100644
--- a/yxi/tests/axi/read-compute-write/seq-mem-vec-add-axi-wrapped.expect
+++ b/yxi/tests/axi/read-compute-write/seq-mem-vec-add-axi-wrapped.expect
@@ -327,7 +327,7 @@ module std_fp_div_pipe #(
         running <= running;
     end
 
-    always_comb begin
+    always @* begin
       if (acc >= {1'b0, right}) begin
         acc_next = acc - right;
         {acc_next, quotient_next} = {acc_next[WIDTH-1:0], quotient, 1'b1};