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

ArrayProxy shape does not equal element shape for a homogeneous array of signed values #476

Closed
pepijndevos opened this issue Aug 15, 2020 · 5 comments
Labels
Milestone

Comments

@pepijndevos
Copy link
Contributor

@pepijndevos pepijndevos commented Aug 15, 2020

https://github.com/nmigen/nmigen/blob/e46118dac0df315694b0fc6b9367d285a8fc12dd/nmigen/hdl/ast.py#L1202
Why is the signedness of the element added to its width? Maybe the intention was to add the "sign bit", but my understanding is that this is purely a marker, and the width is unchanged.

@whitequark whitequark added the bug label Aug 15, 2020
@whitequark
Copy link
Member

@whitequark whitequark commented Aug 15, 2020

Maybe the intention was to add the "sign bit", but my understanding is that this is purely a marker, and the width is unchanged.

That was indeed the intent. Your understanding is correct.

Why is the signedness of the element added to its width?

I think it's just a bug.

@pepijndevos
Copy link
Contributor Author

@pepijndevos pepijndevos commented Aug 15, 2020

Maybe it's not a bug actually. Just really... odd.

So imagine I am weird and I put a signed and an unsigned number in the same array. Now shape will return the maximum of the width, and maximum of signedness. This means that if I have signed 0xff=-1 and unsigned 0xff=255, and I put them in the same array, I think what will happen if you remove the extra bit is that the unsigned one will become -1 instead of 255.

So this poses a larger question: are inhomogeneous elements in an array a thing that is supported?

It seems that this is a very weird thing to want in a HDL, and in this particular case, causes signed array values to be one bit too big.

@whitequark
Copy link
Member

@whitequark whitequark commented Aug 15, 2020

So this poses a larger question: are inhomogeneous elements in an array a thing that is supported?

Hm. They... have to be; if we forbade them, something as simple as Array([-1, 0, 1]) would not work. So maybe this is only a documentation bug.

@whitequark
Copy link
Member

@whitequark whitequark commented Aug 15, 2020

Let's discuss this on the Monday meeting.

@whitequark
Copy link
Member

@whitequark whitequark commented Aug 17, 2020

We discussed this on the meeting and concluded that heterogenous arrays are currently handled correctly, so nothing needs to be (or can be) changed there.

However, homogeneous arrays where all elements are signed are spuriously extended by one bit:

>>> Array([Const(0xff, signed(8))])[Const(0)].shape()
signed(9)

and this should be fixed.

@whitequark whitequark added this to the 0.3 milestone Aug 17, 2020
@whitequark whitequark changed the title Possible bug in ArrayProxy for signed values ArrayProxy shape does not equal element shape for a homogeneous array of signed values Aug 17, 2020
pepijndevos added a commit to pepijndevos/nmigen that referenced this issue Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants