Skip to content
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

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

Open
AndrzejSawula opened this issue Dec 19, 2023 · 7 comments

Comments

@AndrzejSawula
Copy link

AndrzejSawula commented Dec 19, 2023

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 AndrzejSawula changed the title remove overflow warning when using signed base-2 literals remove overflow warning when using signed integer literals of same representation size? Dec 19, 2023
@jnfoster
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Author

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

@jnfoster
Copy link
Collaborator

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

@jonathan-dilorenzo
Copy link
Collaborator

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
Copy link
Contributor

vlstill commented Jan 10, 2024

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.

@jonathan-dilorenzo
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants