-
Notifications
You must be signed in to change notification settings - Fork 141
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
Comments
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:
The modified code:
Fails with:
Why is |
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. |
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.
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. |
Yeah, I agree. I'll consider the right way to fix this. |
Fixed in 0836f13 |
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
Or run the updated unit test added by #877
The text was updated successfully, but these errors were encountered: