This is an archive of the discontinued LLVM Phabricator instance.

[C2x] Implement literal suffixes for _BitInt
ClosedPublic

Authored by aaron.ballman on Mar 1 2022, 2:02 PM.

Details

Summary

WG14 adopted N2775 (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2775.pdf) at our Feb 2022 meeting. This paper adds a literal suffix for bit-precise types that automatically sizes the bit-precise type to be the smallest possible legal _BitInt type that can represent the literal value. The suffix chosen is wb (for a signed bit-precise type) which can be combined with the u suffix (for an unsigned bit-precise type).

The preprocessor continues to operate as-if all integer types were intmax_t/uintmax_t, including bit-precise integer types. It is a constraint violation if the bit-precise literal is too large to fit within that type in the context of the preprocessor (when still using a pp-number preprocessing token), but it is not a constraint violation in other circumstances. This allows you to make bit-precise integer literals that are wider than what the preprocessor currently supports in order to initialize variables, etc.

Diff Detail

Event Timeline

aaron.ballman created this revision.Mar 1 2022, 2:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 2:02 PM
aaron.ballman requested review of this revision.Mar 1 2022, 2:02 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 1 2022, 2:02 PM

Added release notes.

erichkeane added inline comments.
clang/include/clang/Lex/LiteralSupport.h
65

I can't help but wonder if there are some really good opportunities to merge a bunch of these mutually exclusive ones into an enum...

enum class SizeType {
Long,
LongLong,
SizeT,
Float,
Imaginary,
Float16,
Flaot128,
Fract,
Accum,
BitInt

Or something?

clang/lib/AST/StmtPrinter.cpp
1157

Nit: Instead of BT->isUnsigned(), we already have isSigned above.

clang/lib/Lex/LiteralSupport.cpp
903

Do we also have to reject isImaginary/isFract/etc separately?

clang/lib/Sema/SemaExpr.cpp
3987

Do you have some AST test here to see what happens to the value when the width is exceeded?

3995

I think it is already covered, but do we make sure the literal itself and the suffix match? That is: -111uwb is invalid?

clang/test/AST/bitint-suffix.c
21

I think by my reading, wbu is valid as well, right? Can we have 1 test for that?

clang/test/Lexer/bitint-constants.c
14

Ah, oh?! interesting...

134

<3

I'd also like a 'TODO' here when this happens that we have something to make sure that a 129bit/etc literal 'works' as expected.

llvm/lib/Support/APInt.cpp
505

I hope someone better at bitnonsense (@craig.topper ?) could take a look at this.

I'm confused why we have to calculate IsNegative 2x, and why 'getBitsNeeded' needs more work for the 10/36 radix cases, but sufficient does not?

aaron.ballman marked 3 inline comments as done.

Address review comments.

clang/include/clang/Lex/LiteralSupport.h
65

That seems plausible, but perhaps as a follow-up. We're going from 12 bits to 13 with this patch (and correctly a layout issue as a drive-by), so we're not at or near an allocation unit boundary.

clang/lib/AST/StmtPrinter.cpp
1157

Good call!

clang/lib/Lex/LiteralSupport.cpp
903

I'm not super familiar with fixed-point literals, but at least with imaginary, you can make a _Complex from any integer type, including _BitInt, so I don't think there's a reason to reject that case.

clang/lib/Sema/SemaExpr.cpp
3987

Not an AST test -- can't get one of those because this emits an error. But we do have a semantic test, see bitint-constants.c lines 135-136.

3995

We do not, but that's consistent with other suffix confusion: https://godbolt.org/z/rExPGdex3

(Remember, the - isn't part of the literal, that's a unary negation operator applied to the positive literal.)

clang/test/AST/bitint-suffix.c
21

We have one -- bitint-constant.c lines 89-92.

clang/test/Lexer/bitint-constants.c
134

Added a comment.

erichkeane added inline comments.Mar 2 2022, 12:12 PM
clang/include/clang/Lex/LiteralSupport.h
65

SGTM.

clang/lib/AST/StmtPrinter.cpp
1157

Isn't this reversed now?

clang/lib/Sema/SemaExpr.cpp
3995

Oh! TIL!

Correct a think-o that fixes a failing test. Also removed a newline that didn't need to exist.

aaron.ballman marked 5 inline comments as done.Mar 2 2022, 12:24 PM
aaron.ballman added inline comments.
clang/lib/AST/StmtPrinter.cpp
1157

It was, and the AST/bitint-suffix.c test caught it, but I missed it locally. I've corrected here (and verified we have test coverage for the code). Oops!

erichkeane added inline comments.Mar 2 2022, 12:25 PM
clang/lib/AST/StmtPrinter.cpp
1156

You don't need BT here anymore (sorry for missing that!).

aaron.ballman marked 4 inline comments as done.

getAs -> isa

clang/lib/AST/StmtPrinter.cpp
1156

No worries, I should have caught that as well. I've corrected it.

erichkeane accepted this revision.Mar 2 2022, 12:31 PM

Would love someone to check out the APInt changes to confirm it is 'doing the right thing', but as far as I can tell it is right (and I'm just not confident enough to say so :D).

This revision is now accepted and ready to land.Mar 2 2022, 12:31 PM

If there aren't additional review comments, I plan to land this tomorrow. Any comments that come in after that are ones I'm happy to address post-commit.

aaron.ballman closed this revision.Mar 14 2022, 6:25 AM

Thanks for the reviews! I've commit in 8cba72177dcd8de5d37177dbaf2347e5c1f0f1e8 (LLVM bits) and 8cba72177dcd8de5d37177dbaf2347e5c1f0f1e8 (Clang bits)