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

slang incorrectly sizes -2^N constant integrals #878

Closed
udif opened this issue Jan 25, 2024 · 5 comments
Closed

slang incorrectly sizes -2^N constant integrals #878

udif opened this issue Jan 25, 2024 · 5 comments
Labels

Comments

@udif
Copy link
Contributor

udif commented Jan 25, 2024

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

udif commented Jan 25, 2024

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

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: #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
Copy link
Contributor Author

udif commented Jan 25, 2024

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

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

@MikePopoloski
Copy link
Owner

Fixed in 0836f13

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

No branches or pull requests

2 participants