MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
558 stars 122 forks source link

slang incorrectly sizes -2^N constant integrals #878

Closed udif closed 2 months ago

udif commented 5 months ago

Describe the bug slang seems to incorrectly size -2^N integral constants as having N+2 bits instead of N+1 bits.

To Reproduce Simply run

module t;
wire [15:0]x = -16'sd32768;
endmodule

Or run the updated unit test added by #877

udif commented 5 months ago

I was about to close this, because I thought it was due to the assignment to unsigned wire, but then I ran this:

Original code failed with:

t.v:3:22: warning: vector literal too large for the given number of bits (17 bits needed) [-Wvector-overflow]
wire [15:0]x = -16'sd32768;
                     ^

The modified code:

module t;
wire [15:0]x = -16'sd32767;
wire [15:0]y = -16'sd32768;
logic signed [15:0]z = -16'sd32768;
endmodule

Fails with:

t.v:3:22: warning: vector literal too large for the given number of bits (17 bits needed) [-Wvector-overflow]
wire [15:0]y = -16'sd32768;
                     ^
t.v:4:30: warning: vector literal too large for the given number of bits (17 bits needed) [-Wvector-overflow]
logic signed [15:0]z = -16'sd32768;
                             ^

Why is -16'sd32767, whose minimal hex representation is 16'h8001 can be assigned to a 16-bit unsigned wire,
while -16'sd32768, whose minimal hex representation is 16'h8000 can't be assigned to the same 16-bit unsigned wire? Any why can't -16'sd32768 be assign to a 16-bit signed logic?

MikePopoloski commented 5 months ago

You're not getting warnings about the assignment, you're getting warnings about the literal itself. According to the LRM, there is no such thing as a negative literal in SystemVerilog; instead there is a positive literal and a minus operator applied to it. What happens for -16'sd32768 is that the positive number overflows, causing it to become negative, which triggers the warning. Applying the minus operator to that ends up giving you the right answer because the negative of INT_MIN is INT_MIN in two's complement.

There's a bunch more context in this similar issue: https://github.com/MikePopoloski/slang/issues/817

As to why slang issues a warning here... it could work out that you're about to negate the literal and suppress the warning. Other tools issue warnings for similar cases (for example, VCS will warn with 32-bit literals but not based 16 bit literals for some reason). slang takes the approach with being consistent in always warning about literal overflows, valuing consistency over trying to squash particular cases like this. I guess it's open for debate whether that's more annoying than its worth, but the fix is also pretty simple: you can just remove the 's' from the literal.

udif commented 5 months ago

I'll have to closely read those sections mentioning signed decimal constants (e.g. 5.7.12, 11.3.3, 11.4.3.1, 11.7) in the LRM again. Assuming your analysis is correct, it seems we have 3 cases with negative signed numbers (I'm relating to the intended usage, not implying these are really negative signed decimal constants, or negated positive signed constants as you say the LRM implies).

  1. -(2^(N-1)) < n < 2^(N-1) These fit in N bits, positive or negative, and we have no problem with.
  2. n == -2^(N-1) The case in question
  3. n >= 2^(N-1), n < -2^(N-1) These don't fit in N bits in any way you look at it.

At the moment, slang treats case 2 in the same way as case 3.

I'm arguing that given that case 2 is useful, and that -16'sd32768 (as an example for N=16) is the clearest and simplest way to write signed lowest negative integers, perhaps it should be separated into a different warning that can be disabled at will. The alternative is to either turn off all warnings and potentially hide real overflow warnings, or individually turn off all specific instances where this is used, which is tedious.

MikePopoloski commented 4 months ago

Yeah, I agree. I'll consider the right way to fix this.

MikePopoloski commented 2 months ago

Fixed in 0836f136208d37efe5ac0417737004274f99511b