Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#1232 Fix for pr3518267

open
nobody
None
5
2012-04-28
2012-04-16
Martin Whitaker
No

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.

Discussion

1 2 > >> (Page 1 of 2)
  • Regression test for test suite

     
    Attachments
  • Cary R.
    Cary R.
    2012-04-16

    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.

     
    • status: open --> pending
     
  • 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.

     
    • status: pending --> open
     
  • 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.

     
  • Cary R.
    Cary R.
    2012-04-28

    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.

     
1 2 > >> (Page 1 of 2)