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

Width cast of expression not propagating #78

Open
dpetrisko opened this issue Jan 11, 2025 · 8 comments
Open

Width cast of expression not propagating #78

dpetrisko opened this issue Jan 11, 2025 · 8 comments
Labels
bug Something isn't working

Comments

@dpetrisko
Copy link
Contributor

Hello! I've found an apparent discrepancy in a basic synthesis. This is a minimized testcase when trying to synthesize https://github.com/bespoke-silicon-group/basejump_stl/blob/master/bsg_misc/bsg_decode.sv (in a larger design).

testcase1.tcl uses read_slang whereas testcase2.tcl uses read_verilog. The output is a $shl in both, but the width in the slang version is only a single bit instead of multi-bit. The output width is corrected by 0-padding so there is no warning, but the functionality is broken.

testcase.sv:

 module testcase
  #(localparam width_gp = 8
    , localparam lg_width_gp = $clog2(width_gp)
    )
   (input logic [lg_width_gp-1:0] in
    , output logic [width_gp-1:0] out
    );

   wire [width_gp-1:0] lo;
   wire [lg_width_gp-1:0] li;
   assign li = in;
   assign lo = (width_gp) ' (1'b1 << li);
   assign out = lo;

 endmodule

testcase1.tcl:

 yosys -import
 plugin -i slang
 yosys -import

 read_slang \
     testcase.sv \
     -top testcase \
     --ignore-initial \
     --ignore-timing \
     --ignore-unknown-modules \
     --best-effort-hierarchy

 hierarchy -check -top testcase

 write_verilog -nostr -noattr -noexpr -nohex -nodec testcase1.out.v

testcase1.out.v:

 /* Generated by Yosys 0.48+57 (git sha1 d37449605, g++ 13.3.0-6ubuntu2~24.04 -fPIC -O3) */

 module testcase(in, out);
   wire _0_;
   input [2:0] in;
   wire [2:0] in;
   wire [2:0] li;
   wire [7:0] lo;
   output [7:0] out;
   wire [7:0] out;
   \$shl  #(
     .A_SIGNED(32'b00000000000000000000000000000000),
     .A_WIDTH(32'b00000000000000000000000000000001),
     .B_SIGNED(32'b00000000000000000000000000000000),
     .B_WIDTH(32'b00000000000000000000000000000011),
     .Y_WIDTH(32'b00000000000000000000000000000001)
   ) _1_ (
     .A(1'b1),
     .B(li),
     .Y(_0_)
   );
   assign li = in;
   assign lo = { 7'b0000000, _0_ };
   assign out = lo;
 endmodule

testcase2.tcl:

 yosys -import

 read_verilog \
     -sv \
     testcase.sv

 write_verilog -nostr -noattr -noexpr -nohex -nodec testcase2.out.v

testcase2.out.v

 /* Generated by Yosys 0.48+57 (git sha1 d37449605, g++ 13.3.0-6ubuntu2~24.04 -fPIC -O3) */

 module testcase(in, out);
   wire [7:0] _0_;
   input [2:0] in;
   wire [2:0] in;
   wire [2:0] li;
   wire [7:0] lo;
   output [7:0] out;
   wire [7:0] out;
   \$shl  #(
     .A_SIGNED(32'b00000000000000000000000000000000),
     .A_WIDTH(32'b00000000000000000000000000000001),
     .B_SIGNED(32'b00000000000000000000000000000000),
     .B_WIDTH(32'b00000000000000000000000000000011),
     .Y_WIDTH(32'b00000000000000000000000000001000)
   ) _1_ (
     .A(1'b1),
     .B(li),
     .Y(_0_)
   );
   assign li = in;
   assign lo = _0_;
   assign out = lo;
 endmodule

diff.txt:

 <   wire _0_;
 ---
 >   wire [7:0] _0_;
 16c16
 <     .Y_WIDTH(32'b00000000000000000000000000000001)
 ---
 >     .Y_WIDTH(32'b00000000000000000000000000001000)
 23c23
 <   assign lo = { 7'b0000000, _0_ };
 ---
 >   assign lo = _0_;
@dpetrisko
Copy link
Contributor Author

Appears to be a casting issue, as this patch produces identical output in testcase1 / testcase2):

< assign lo = (width_gp) ' (1'b1 << li);
--
> assign lo = (width_gp) ' (8'b1 << li);

I believe that the constant should be promoted, but I'll check the SV spec to see if yosys is just too generous :)

@dpetrisko dpetrisko changed the title Error reading basic decoder Width cast of expression not propagating Jan 11, 2025
@povik povik added the bug Something isn't working label Jan 11, 2025
@povik
Copy link
Owner

povik commented Jan 11, 2025

Indeed!

So far I've found:

  • Verific sides with read_verilog
  • this originates from slang since it resolves the result of the shift operation to be one bit wide
  • I have confirmed this bug persists in the current version of slang (we're lagging behind with the version which is vendored inside yosys-slang)

I will make a report upstream and/or look for a fix later.

@dpetrisko
Copy link
Contributor Author

dpetrisko commented Jan 11, 2025

Thanks! As an additional data point, read_verilog works without the cast so presumably the LHS makes an implicit cast. I thought verilator might have lint problems without the cast, but seems okay with it

@povik
Copy link
Owner

povik commented Jan 11, 2025

As an additional data point, read_verilog works without the cast

read_verilog and read_slang both, right? So it's only with the cast that there seems to be an issue with yosys-slang

@povik
Copy link
Owner

povik commented Jan 11, 2025

The text in 1800-2023 about these kinds of casts on p. 140-141 is conflicting to me.

First it says

If the casting type is a constant expression with a positive integral value, the expression in parentheses shall
be padded or truncated to the size specified. It shall be an error if the size specified is zero or negative.

but further down it goes on to say

When changing the size, the cast shall return the value that a packed array type with a single [n-1:0] dimension would hold after being assigned the expression, where n is the cast size.

Still the second quote is more specific, and it's how other tools seem to interpret the cast, so I expect that's the right interpretation.

@dpetrisko
Copy link
Contributor Author

dpetrisko commented Jan 11, 2025

read_verilog and read_slang both, right? So it's only with the cast that there seems to be an issue with yosys-slang

just read_verilog! read_slang still does 1b

EDIT: nope

@povik
Copy link
Owner

povik commented Jan 11, 2025

just read_verilog! read_slang still does 1b

That's not what I see. If I read:

 module testcase
  #(localparam width_gp = 8
    , localparam lg_width_gp = $clog2(width_gp)
    )
   (input logic [lg_width_gp-1:0] in
    , output logic [width_gp-1:0] out
    );

   wire [width_gp-1:0] lo;
   wire [lg_width_gp-1:0] li;
   assign li = in;
   assign lo = (1'b1 << li);
   assign out = lo;

 endmodule

and use testcase1.tcl, I get

/* Generated by Yosys 0.48+47 (git sha1 UNKNOWN, clang++ 19.1.5 -fPIC -O3) */

module testcase(in, out);
  wire [7:0] _0_;
  input [2:0] in;
  wire [2:0] in;
  wire [2:0] li;
  wire [7:0] lo;
  output [7:0] out;
  wire [7:0] out;
  \$shl  #(
    .A_SIGNED(32'b00000000000000000000000000000000),
    .A_WIDTH(32'b00000000000000000000000000001000),
    .B_SIGNED(32'b00000000000000000000000000000000),
    .B_WIDTH(32'b00000000000000000000000000000011),
    .Y_WIDTH(32'b00000000000000000000000000001000)
  ) _1_ (
    .A(8'b00000001),
    .B(li),
    .Y(_0_)
  );
  assign li = in;
  assign lo = _0_;
  assign out = lo;
endmodule

@dpetrisko
Copy link
Contributor Author

dpetrisko commented Jan 12, 2025

That's not what I see. If I read:

ignore me; you're right. I was looking at stale output. Rerunning, I see correct output.

Interestingly, there is a slight difference in the netlist without the cast. read_verilog has the A_WIDTH of the shifter at width=1, whereas read_slang has A_WIDTH at width=8. Probably this will get optimized at a future constant prop pass, but flagging that the internal representation may be different between the two right after reading

 13c13
 <     .A_WIDTH(32'b00000000000000000000000000000001),
 ---
 >     .A_WIDTH(32'b00000000000000000000000000001000),
 18c18
 <     .A(1'b1),
 ---
 >     .A(8'b00000001),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants