Refactor `std_fp_div_pipe` primitive to avoid Verilog features not supported by Icarus

[KarlJoad]

It seems that portions of Calyx's standard library written in SystemVerilog is not able to be simulated by Icarus.

[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.

[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:

/* 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

[sampsyo]

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.

[KarlJoad]

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 does something similar to what is done with the bit concatenation with the constant, but using SystemVerilog's case instead.

[KarlJoad]

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:

$ iverilog -g2012 test.v

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:

$ 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.

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:

$ iverilog -g2012 test.v

$ echo $?
0

[rachitnigam]

You can probably rewrite this to used assign instead of the always_comb:

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 ? ... : ...;

[KarlJoad]

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 between always_comb and always @*; I am not sure how Calyx's surrounding infrastructure will fare with this kind of change.

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

$ iverilog -g2012 test.v

$ echo $?
0

[rachitnigam]

Calyx does not emit always_comb so the only uses are in the standard library.

[KarlJoad]

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 @*.

[rachitnigam]

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.