p4lang / p4-spec

Apache License 2.0
178 stars 80 forks source link

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

Open AndrzejSawula opened 11 months ago

AndrzejSawula commented 11 months ago

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?

jnfoster commented 11 months ago

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.

AndrzejSawula commented 11 months ago

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 commented 11 months ago

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

jnfoster commented 11 months ago

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

jonathan-dilorenzo commented 10 months ago

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?

vlstill commented 10 months ago

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.

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.

jonathan-dilorenzo commented 10 months ago

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.