p4-spec icon indicating copy to clipboard operation
p4-spec copied to clipboard

remove overflow warning when using signed integer literals of same representation size?

Open AndrzejSawula opened this issue 1 year ago • 7 comments

Should the compiler emit warnings when signed integer values are initialized using their literal bit representations?

Section 6.4.3.2 specifies that one can specify an integer literal using its bitwise representation, and if the literal type is signed, the two's complement interpretation is used: 8s0b1010_1010 // an 8-bit signed number with value -86

But then section 7.1.6.6 deals further with integer literal types, giving the following examples:

...
2s3	Type is int<2>, value is -1 (last 2 bits), overflow warning
1w10	Type is bit<1>, value is 0 (last bit), overflow warning
1s1	Type is int<1>, value is -1, overflow warning

While the second-to-last example is a clear overflow case, the other two do not seem to be – no bit is being discarded, it is rather reinterpretation than overflow (unless one includes an extra leading zero bit in the required representation of "signed 3"). Specifying the values in base-2 (e.g. 2s0b11 for -1) should generate warnings as well, and currently it does. Is this desirable?

AndrzejSawula avatar Dec 19 '23 12:12 AndrzejSawula

I agree that the comments in the spec look wrong. Do you want to submit a PR fixing it? We can discuss it at the January LDWG meeting.

jnfoster avatar Dec 20 '23 11:12 jnfoster

I agree that the comments in the spec look wrong. Do you want to submit a PR fixing it? We can discuss it at the January LDWG meeting.

Sure, I'll prepare the PR.

AndrzejSawula avatar Dec 20 '23 14:12 AndrzejSawula

Please see p4-spec PR #1270 and p4c PR #4309.

AndrzejSawula avatar Dec 22 '23 17:12 AndrzejSawula

Thanks! We'll discuss this at January's LDWG.

jnfoster avatar Dec 27 '23 15:12 jnfoster

Hi @asawulaINTC, we discussed this in the January LDWG meeting and are basically concerned that this change is likely to make the readability worse than it is currently. We agree that there isn't otherwise a way to define a negative integer literal, but that is by design in the specification. We'd propose these alternative solutions, but would also love to hear if you have a case where those don't work as well. In both, you would first construct an arbitrary-size integer and negate it with unary negation. Then:

  1. Assign it to the signed integer that you wanted to give this value to in the first place.
  2. Explicitly type-cast it to a signed integer with the appropriate number of bits. This solution makes more sense than the first for e.g. constructor parameters.

We think both of these should be significantly clearer to a reader. Do they not work in the situation you have in mind, and if so why?

jonathan-dilorenzo avatar Jan 10 '24 17:01 jonathan-dilorenzo

One thing that we did not realize when discussing the spec is that the warning is actually emitted also for cases that are perfectly valid.

const int<2> val = -1;
hdr.h0.v0 = (bit<8>)(bit<2>)val;

-->

main.p4(42): [--Wwarn=mismatch] warning: -2w1: negative value with unsigned type
        const int<2> val = -1;
                            ^

However, I am pretty sure this is not a mistake in spec, this is a mistake in implementation. And also, the warning is very confusing, the problem is not in fact the first assignment. The problem is in the cast below it and the warning is emitted while constant folding.

  • first, val is substituted, getting new subexpr (bit<2>)-1
  • then, while constant folding this expr, the compiler attempts to create a new constant and calls essentially new Constant(IR::Type_Bits::get(2, false) /* bit<2> */, -1)
  • the call emits a warning, because a now-unsigned constant is being initialized by -1.

I think that we should probably disable warnings in all the Constant calls that arise in constant folding -- these are problems not arising from the user's code, either there was something risky and then it should have already been reported, or this is precomputation that should not emit warnings.

vlstill avatar Jan 10 '24 19:01 vlstill

Yeah, so I don't see a place where the spec suggests this warning should be emitted at that spot. Probably it would be correct to emit the warning at the second line here? Casting a negative number to an unsigned type probably does deserve a warning.

jonathan-dilorenzo avatar Jan 10 '24 20:01 jonathan-dilorenzo