This is an archive of the discontinued LLVM Phabricator instance.

clang: Introduce -fexperimental-max-bitint-width
ClosedPublic

Authored by mgehre-amd on Jun 8 2022, 3:53 AM.

Details

Reviewers
aaron.ballman
Summary

This splits of the introduction of -fexperimental-max-bitint-width
from https://reviews.llvm.org/D122234
because that PR is still blocked on discussions on the backend side.

I was asked [0] to upstream at least the flag.

[0] https://github.com/llvm/llvm-project/commit/09854f2af3b914b616f29cb640bede3a27cf7c4e#commitcomment-75116619

Diff Detail

Event Timeline

mgehre-amd created this revision.Jun 8 2022, 3:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 3:53 AM
mgehre-amd requested review of this revision.Jun 8 2022, 3:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 3:53 AM

Given that this is a hidden option we expect to remove... do we add a release note for it? I think we probably should (for user awareness in case more users are in the same situation as the original reporter), and that gives a place to document that we will be removing the option in the future.

clang/include/clang/Basic/LangOptions.def
448

Should we add a comment about how this is expected to be removed in the future once the backend work is fixed?

clang/include/clang/Basic/TargetInfo.h
239

Do we want to make this into an Optional<unsigned> so we don't have to use 0 as a sentinel value? (I don't feel strongly, but we already use Optional in this interface anyways.)

clang/include/clang/Driver/Options.td
6088

Maybe?

clang/lib/Serialization/ASTReader.cpp
317

Good catch!

clang/test/Sema/large-bit-int.c
1

Thoughts on adding a frontend diagnostic if the user specifies a value larger than llvm::IntegerType::MAX_INT_BITS? I'm on the fence. OTOneH, this gives a good user experience in the event of mistypes in the command line, OTOtherH, it's a cc1-only option that we expect to remove in the (hopefully) near future.

Also, I don't think the triple is needed and I'm pretty sure -Wno-unused is the default already.

mgehre-amd updated this revision to Diff 435161.Jun 8 2022, 7:53 AM
mgehre-amd marked 4 inline comments as done.

Used Optional
Added entry to release notes
Modifed help text according to suggestion
Added comment to LANGOPT that it will be removed in the future

Thanks for the quick review!

clang/test/Sema/large-bit-int.c
1

I was thinking that this is both only a cc1 option and at the same time MAX_INT_BITS is really big, so I don't imagine a practical case where one intends to have bigger _BitInts.

mgehre-amd marked an inline comment as done.Jun 8 2022, 7:54 AM
aaron.ballman accepted this revision.Jun 8 2022, 9:00 AM

LGTM, thank you!

clang/test/Sema/large-bit-int.c
1

Perfect, I'm sold. We can always add a diagnostic later if we find users actually run up against this.

This revision is now accepted and ready to land.Jun 8 2022, 9:00 AM