-
Notifications
You must be signed in to change notification settings - Fork 175
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
[RFC] Shape Conversion Operators Round 2 #464
Comments
Switching hats, I'm personally strongly in favor of Proposal 1. My mental model is that |
cc @programmerjake -- it was your comment which had the insight about preserving mathematical value that led to a second round of this RFC, so I'd certainly like to know your opinion here as well cc @Ravenslofty -- you had some good arguments against the previous round of the RFC, so I'd like to know what you think about this one |
I think proposal 1 is the best option. The use of a shape as a target size in prop 2 and 3 makes me uncomfortable: consider somebody who knows that |
To add to @Ravenslofty's comment (without necessarily agreeing with it): although it might seem that the problem of So in case of |
All of the stuff regarding preserving mathematical value looks good to me. I disagree with proposal 1 having overflow checks for truncation, I may be somewhat biased by my experience with C, C++, Rust, Java, JavaScript, LLVM IR, D, and probably a few other programming languages that also wrap, but I think truncation should wrap modularly instead of cause an error. If desired, there can be a If truncation is changed to wrap when not using explicitly Regarding casting to a shape and issues with |
I agree (I missed this when reading the RFC text the first time). If nothing else, this is inconsistent with pretty much any other part of the language we have at the moment. Introducing
We discussed this on IRC. Although exceptions could be added, this seems like it would make writing generic (e.g. over a user-specified enum) code harder, which would be counterproductive. |
I agree here, I forgot to mention it in my above comment but in general truncation removing the required number of bits, starting with the MSB, regardless of their value, seems to be the semantics we want. I would want this to be a lint. |
There... are no truncation overflow checks proposed? Aside from a guard against truncating to a wider value than what you started with. As for |
@Ravenslofty programmerjake refers to this text:
which came along for the ride from the comment on #381 referenced in the RFC. |
I was thinking it would lint against all enums, not just non-power-of-2 enums, though I can understand why linting against enums or ranges seems like a bad idea. |
Ah right, so I'm fine with linting against all enums, but linting against only non-power-of-2 ranges has the same hazard. |
I like proposal #2. Proposals #1 or #3 seem fine, with a caveat in that I don't like the use of the term "truncate" to indicate chopping bits off the MS end of a signal - to me truncation implies dropping LS bits (as in mathematical truncation which reduces precision) and I suspect this could cause some confusion. Proposal #2 seems better to me because it has the same effect as 1 or 3 but avoids the use of the confusing term. |
The use of "truncate" in the sense of cutting off MSBs has some precedent, e.g. see LLVM LangRef. |
Fair. Coming from a primarily hardware / DSP background my definition of truncation is likely somewhat narrower in scope than what's used in the wider software engineering world. Regardless, I'm glad to see these capabilities considered. |
I think it's a fair objection! Since hardware and DSP are both something that I would expect people interested in nMigen to know, maybe |
I agree, that's valuable feedback - it never even occurred to me that |
I asked the Digital Design Discord I'm in this question:
The general consensus was that the intuitive answer was "the 16 least significant bits", but that |
Looking at some DSP I wrote in nmigen, I actually use slicing more frequently to select the msbits of some signals than the lsbits, i.e. I'm doing This is a problem with all three proposals in so far as they all make signals shorter by always discarding the msbits, which is maybe an argument against the "universal" That aside I have a weak preference for 3 over 1, since we already store signedness in the signal shape and for example right shift is arithmetic for signed signals and logical for unsigned signals. Could we just take all the proposals as described, so we have |
one more thing to consider is casting negative numbers to unsigned shapes. a common trick in C to set all bits of an unsigned number is to assign -1 to it- and that is equivalent to assigning the max value of that type. I think we should have the same in nmigen - so |
Something this RFC should take into account is the issue highlighted in #481, which we definitely should address as a part of any new set of shape conversion operators. I believe that it is an argument in favor of Proposal 1: if we allow |
There has not been a consensus on this proposal, and due to the complexity and impact it would have to go through our new RFC process to be accepted. In addition, few people have expressed a need for this functionality, so perhaps it is just not particularly needed. |
This RFC is a continuation of #381 .
Currently, in order to change the shape of a signal, the following workarounds are needed:
v[:new_width]
v | C(0, new_width)
.This code is confusing when it is encountered among a bunch of arithmetic operations, and it's more of a side effect than an explicit request to change shape. This makes it fragile, because neither of these operations is guaranteed to return a value with width
new_width
.There are three proposals on the table currently for improving this situation.
Proposal 1: Add three new operators, each of which takes a
width
v.zero_extend(new_width)
that works only on unsigned values, requiresnew_width >= len(v)
, and returns unsigned values,v.sign_extend(new_width)
that works only on signed values, requiresnew_width >= len(v)
, and returns signed values,v.truncate(new_width)
that works on values of either signedness, requiresnew_width <= len(v)
, returns a value of the same signedness as the input, and checks (in simulation and formal) that the truncated bits were all identical to 0 (for unsigned values) or the new MSB (for signed values).Proposal 2: Add a single general-purpose shape conversion operator,
v.as(new_shape)
, for truncation, zero/sign extension, and type conversion. The new operator would extend v ifnew_shape.width > len(v)
, truncate v ifnew_shape.width < len(v)
, and return a value with shapenew_shape
. The semantics of this operator would be to preserve the mathematical value (modulo output size), as described in this comment.Proposal 3: Add two new operators, each of which takes a
Shape
v.extend(new_shape)
that works on signed or unsigned values and requiresnew_shape.width >= len(v)
v.truncate(new_shape)
that works on values of either signedness and requiresnew_shape.width <= len(v)
Both of these operators would also preserve the mathematical value modulo output size and return a value with shape
new_shape
.The text was updated successfully, but these errors were encountered: