-
Notifications
You must be signed in to change notification settings - Fork 177
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
Signed math on Cat gives incorrect results #473
Comments
Good catch! |
I thought about it a bit more, and actually the result is obviously sign-extended, but I believe the issues is related to the width of the result. The result of negating an 8-bit number is a 9-bit number, and the result we get from the Is there an easy way to confirm this bug is specific to pysim? I see there are many backends, but not sure how easy it is to simulate their output from within Python, or you'd have to write say a whole Verilog testbench and run it in verilator. |
The only place where this happens is here: https://github.com/nmigen/nmigen/blob/73f672f57c606ac29087ffb09c43a7e4d9a9dfc6/nmigen/hdl/ast.py#L678-L679
This. |
I added some debugging:
I believe this mask originates here I'm not sure exactly what is going on with the masking and what would be the correct solution. |
Looks like |
What effect is I don't think the masking is fine. It's basically masked to 8 bits, then negated, and then 9 bits of the result are used, leaving the MSB inverted. But maybe something else is missing. |
None; exactly as the comment says. Treating the bits as signed or unsigned doesn't change the bits, it changes how the bits are treated by arithmetic operations that follow.
The masking code performed by |
(The bug was in the implementation of negation.) |
Thank you))) |
Consider the following minimal example:
Which prints
Of course python's binary representation is a bit misleading here.
-0b11
should actually be0b111...1101
in two's complement.So actually
-0b11111101
would be0b1111...1100000011
IIRC, which is 3 except not sign-extended.So it appears that when you put a signed number in a
Cat
and convert that to signed, the result is not sign-extended correctly.The text was updated successfully, but these errors were encountered: