Menu
▾
▴
#1232 Fix for pr3518267
We want unsized literals to behave as if they have infinite size, so need to force them to be zero extended, even if the expression type is signed.
This patch is for devel only, and is based on the reworked expression elaboration code, so will not backport easily. v0.9 has a number of known issues with expression types and widths, so I'm inclined to leave well alone.
Also attached is a regression test for inclusion in the test suite. This doesn't need any special flags or a gold file.
Patch for compiler
Regression test for test suite
I don't think this is the correct fix though I have not generated a case that breaks it. My concern is that we are effective casting some signed values to unsigned and that this will adversely effect expression code that uses the value. I'll try to look for a case that breaks later.
The key point is that literal integer numbers can never be negative. This is explicitly stated in the standard - a leading minus sign should be treated as a unary minus operator. So as we want to treat unsized literals as being infinitely extendable, they should always be handled as if they had a leading zero bit, even if they are declared to be of signed type. Declaring a literal to be of signed type merely affects the propagated expression type.
Note that the expression type has been calculated before PENumber::elaborate_expr is called, and the NetEConst object is cast to the correct type once the number has been extended to the required width.
I think the patch is not quite right. In particular, although I agree that the sign bit may be wrong without this patch (it should indeed be zero) but the expression is still signed, so marking it unsigned is wrong. I think the proper fix is to pad it much farther upstream, possibly in the lexor.lex file where the signed/unsigned/sized/unsized rules are detected.
The literal value is temporarily marked as unsigned to force zero extension:
val.has_sign(val.has_len() && signed_flag_);
val = cast_to_width(val, expr_wid);
But immediately after this, the resulting NetEConst is correctly typed:
NetEConst*tmp = new NetEConst(val);
tmp->cast_signed(signed_flag_);
If you can come up with a test case that fails, I'll think again. But I believe this patch is correct.
What would you consider to be the correct output from
$display("%h", 'shffffffff);
If we do the padding upstream, it will gain a leading zero. You could certainly make an argument that this would be correct given the way Icarus handles unsized literals, but it might surprise some users.
Note that this example shows up a bug in the current implementation - but that's another issue.
I had a free hour in the airport yesterday and created a patch that fixes this at the source. I added a method to the verinum class that for unsized signed numbers will add a leading zero when needed. This is then called in the parser for unsized based numbers. This fixed the original test case and keeps the original value signed.
FYI my concern with the original patch is not that the simulation will be incorrect but that the other back end code (vlog95 and VHDL) will generate misleading/incorrect code.
Martin your example brings up a good point. Maybe we need to discuss this in more detail on iverilog-devel.
I don't remember if casting the NetEConst is enough to get the underlying verinum to be signed again or not, maybe we need to have a temporary that is used to cast the base verinum back to signed after being extended. I don't think we can just change cast_to_width() and I'm still concerned that the current patch may also effect other unsized signed constants. It should only effect unsized signed based constants.
This is what I think:
The value 'shffff is SIGNED and should be sign (1) extended.
The value 17'shffff is SIGNED and should be sign (0) extended.
The value 'sh7fff is SIGNED and should be sign (0) extended.
In all these cases, the actual extension is not done until the desired length is known from the context. But in all these cases, the lexor should leave the value with the correct sign bit on signed/unsized numbers, even if that means adding a bit in the lexor.
The issue with that thinking is that 's not how other simulators do it since they assume unsized values are 32 bits. That's the basis for the original bug report. It's almost like we need another flag so that unsized signed based values are zero sign extended only when extending is requested. The based part is what the current verinum implementation does not have. We were trying to give consistent functionality even for constants larger than 32 bits.
I plan to look to see if Martin's patch breaks other unsized values, but I already have too much to deal with today so it may not happen anytime soon.
And is in direct conflict with the standard, which requires that unsized numbers are at least 32 bits (section 3.5.1).
If you want to go down this route, I will want to extend the -gstrict-expr-width option to cover this as well. Code portability is important to me.
Icarus Verilog actually tries to get literals to be lossless up to infinite length, so the minimization should only cause literals to be stored in smaller space. The elaboration should understand unsigned numbers to be big enough for the expression to prevent overflow. This is consistent with the "at least 32bits long" in that that behavior is a subset of the more complex behavior.
My reason for going through all the pain of that behavior is that user expect an unsized number to be BIGGER then their sized vectors in an expression. For example, ((a + b + 'd1)/2) should not overflow, even if (a) and (b) are 33bit numbers. If unsized numbers are treated as exactly 32bits, then the expression willl overflow, but if unsized numbers are treated as large enough to prevent loss (and at least 32 bits) in an expression, that that expression will not overflow.
So those are the rules for Icarus Verilog handling of unsized numbers. They are an indeed an extension over basic Verilog, but a legal extension at it shouldn't violate the "at least 32 bits" rule.
OK, those are the rules I've been following. The problem I had was with your example:
"The value 'shffff is SIGNED and should be sign (1) extended."
Following the rules, 'shffff is a signed number with a value of +65535. So the value should be zero extended if used in an expression wider than 16 bits. This maintains compatibility with tools that treat unsized as fixed width.
The logical extension to this (that Cary argued for in pr3518267, and I agreed with) is that the value 'shffffffff should also be zero extended if used in an expression wider than 32 bits. Here we lose compatibility with tools where integers are 32 bits, but maintain compatibility with tools where integers are more than 32 bits (if there are any such tools).
As an aside, the Icarus Verilog behaviour is dangerous if it is being used as the sole means of verifying synthesisable code. All synthesis tools I know of treat unsized literals as exactly 32 bits. So if a user does write code like (a + b +1)/2 where the numerator can overflow 32 bits, there will be a synthesis vs. simulation mismatch.
"The value 'shffff is SIGNED and should be sign (1) extended."
Are you taking note of the "s" in there? Have you verified with other tools? For constants like this that are less then 32bits, there should be no confusion and perfect tool compatibility so it is worth double-checking.
Given all that, where are we WRT the patch that was submitted? Should I apply it?
The value is signed, but I believe Martin has already confirmed that it is not sign extended since it only sets the lower 16 bits of the default 32 bit integer width.
I believe this patch should not be applied. The patch I was working on is also broken. I think I know what needs to be done, but between work and home I've had negative free time lately.