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] Add a (more) general shape conversion operator #381
Comments
TL;DR: Proposal 2, but this might be Stroustrup's rule in effect. My issue with Prop 1 is that I generally don't declare values as signed or not, but instead consider them signed at point of use, because I don't generally need signed math much, so it's an exceptional case. So when I'm wandering through a codebase, something like m.d.sync += imm.eq(i_imm.extend(32)) makes me immediately ask "is this zero or sign-extending?", so I scurry back to its definition (some editors can just point to the definition, some can't) to find i_imm = Signal(signed(12)) and oh, right, it's sign extending. m.d.sync += imm.eq(i_imm.extend(signed(32))) is perhaps more verbose, but it tells me at point of use this is sign-extending. To me it feels like a fundamental tenet of nMigen that things should not silently truncate, so if you're in the middle of some generic code that operates on a width given by a parameter, I would much prefer having to explicitly check and use the right operator than stare at my code wondering if m.d.sync += imm.eq(i_imm.as(signed(32))) is truncating or extending the value. |
@ZirconiumX
This is a good point---right no none of our operators silently truncate, and that is indeed a wonderful property that should be preserved. Of course, the I suppose this soundly excludes Proposal 3.
This is actually not what I proposed. What I proposed results, for a
But what you describe as a good feature here is essentially:
Now, I think the semantics you like is definitely more useful. But there are two problems with it. First, it's inherently ambiguous with the semantics I suggested. However, if it turns out that everyone intuitively understands Second, and more importantly, it makes nMigen's value promotion rules twice as complicated. Right now, if you have We could rescue it by saying that |
Perhaps this points to shape being the wrong thing to disambiguate sign and zero extension. Does I fully agree that the operators should take shape because of the flexibility here, but perhaps it should look something like |
Indeed! I somehow forgot to document that (which worked out for the better), but this was actually my #1 concern about that proposal. I suppose this soundly excludes Proposal 2.
If |
At this point the solutions I see are:
It's hard to argue in favor of Proposal 1 because it ditches shapes. You could try to say that you treat |
I agree with @ZirconiumX in that I think of sign/zero extension as an operation not related to the "type" of the operand(s). My main concern with the I would like a variant of proposal 2, which is ETA: I suppose this brings these functions back into the realm of "bit sequence operations". |
Anything you put into a |
The problem is that we have two things here: the type of extension (i.e. what bit is used for padding in this operation) and the shape of the result (i.e. what bit is used for padding in following operations), and it's not clear what sort of API makes it clear what's going on with both. |
This is going to be ugly, but I'll throw it out there: what if we don't combine the two? We have |
It's kind of ugly, but not that much. I've proposed something similar myself in #381 (comment).
Is there a typo? Are you sure this is the right order? |
Ah, yes, it should be the other way round. |
Per IRC discussion it seems that the most reasonable way forward with this is to have:
|
Is it too ugly to make the new width a parameter of That way you just do: x = Signal(32)
x.as_signed() # keeps the same functionality that the method already has
x.as_signed(4) # truncates
x.as_signed(64) # extends
This will remain that way, but I'm not sure if that's bad. If you change the width to something that is not the correct number you expected (say 3 instead of 32) you'll have a hidden bug anyway. It'd not matter if the new width is higher or lower to the old one. |
I think nMigen generally avoids magic numbers like this, so if this was the accepted syntax it would likely look more like I have two objections to this, which I've already raised in this thread:
But let's be constructive here: if the goal is to distinguish between truncation and extension, we could change the keyword arguments here to something like Okay, back to objecting. What is the semantic order here? Does it convert the value to signed and then extend, or extend and then convert to signed? That matters because if you have an unsigned value as input, the former sign-extends, and the latter zero-extends. What myself and WQ concluded was that it was difficult to specify a "fused" operator that is both concise and clear. This was the reasoning that shot down Proposal 2. |
You're right
I don't consider a change in a width a "silent" truncation, but I understand that it could lead to more hidden bugs than the other way, so I like this approach.
It should first convert to signed and then extend. The intention with
I found this clear with your proposal about using kw args, but of course you can disagree :). I thought this could be a better alternative than adding two new operators with a very limited use, since that also seemed to be a concern here. |
I would argue that the best course of action is to have the extend/truncate operators preserve the mathematical value during conversion, where if the true mathematical value is out of range, then it wraps modularly. This would mean that extend with sign conversion would always extend using the source's signedness then convert signedness. Both ways are identical for truncation. The above is the same method used in C/C++, Rust and probably most other programming languages. I'm not aware of a single programming language that converts signedness then extends. |
Maybe I'm missing something so please correct me if I'm wrong, but if you want an operation that keeps the signedness of the signals you'll already get the expected behavior by just doing the assignment ( This methods that don't depend on the original signal signedness are useful for the use mode that @awygle and @ZirconiumX exposed, where that signedness is only considered at point of use if the operation requires it. The advantage of not depending on the signal declaration is that you don't always have an easy access to it (for example if you are working with standard interfaces and you don't want to duplicate every interface with a signed and an unsigned version).
You may have a point there. Yet, I'm not sure if that's a fair comparison, since a width conversion in hdl should not necessarily match a cast behavior of a programming language where you have fixed data types. |
Just to double check, do you agree that this is the semantics I propose in #381 (comment) ? |
What you described should work and I'm fine with it, though I had intended the explanation of preserving the mathematical value to define what happens for something more like |
On the previous meeting, we decided that @programmerjake's latest comment about preserving mathematical value added enough insight to the conversation that the RFC would have to be rewritten with that in mind. The new RFC is #464, and I'm closing this one in favor of that. |
In response to #292 we've added operators that reinterpret a value of any signedness as a signed or unsigned value of the same width. It would be nice to have an operator that changes the width of a value, too.
Current workaround: for truncation, use
v[:new_width]
; for extension, use a no-op bitwise operation, likev | C(0, new_width)
.new_width
.The workaround seems unacceptable to me.
Proposal 1: add two new operators,
v.trunc(new_width)
for truncation andv.extend(new_width)
for zero/sign extension depending on the signedness ofv
. The new operators would make it an error to truncate to a larger width, or extend to a smaller width than the width ofv
.Proposal 2: like Proposal 1, but the operators accept a
new_shape
rather thannew_width
, enforcing the same preconditions. The signedness of the result matches the signedness of the shape.as_unsigned
/as_signed
can be implemented in terms of either.x.eq(y)
--still isn't expressible and requires an intermediate wireProposal 3: add one new operator,
v.as(new_shape)
for truncation, zero/sign extension, and type conversion. The new operator would extendv
ifnew_shape.width > len(v)
, truncatev
ifnew_shape.width < len(v)
, and return a value with shapenew_shape
.as_unsigned
/as_signed
.Proposal 4: Proposals 2 and 3 at the same time
Thoughts?
The text was updated successfully, but these errors were encountered: