Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor std_fp_div_pipe primitive to avoid Verilog features not supported by Icarus #2253

Closed
KarlJoad opened this issue Aug 7, 2024 · 8 comments · Fixed by #2330
Closed

Comments

@KarlJoad
Copy link

KarlJoad commented Aug 7, 2024

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 sampsyo changed the title Calyx SystemVerilog Standard Library Cannot be Simulated by Icarus Verilog Refactor std_fp_div_pipe primitive to avoid Verilog features not supported by Icarus Aug 8, 2024
@sampsyo
Copy link
Contributor

sampsyo commented Aug 8, 2024

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
Copy link
Author

KarlJoad commented Aug 9, 2024

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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

@KarlJoad
Copy link
Author

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants