Page MenuHomePhabricator

[C2x] Add BITINT_MAXWIDTH support
ClosedPublic

Authored by aaron.ballman on Jan 13 2022, 10:38 AM.

Details

Summary

Part of the _BitInt feature in C2x (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2763.pdf) is a new macro in limits.h named BITINT_MAXWIDTH that can be used to determine the maximum width of a bit-precise integer type. This macro must expand to a value that is at least as large as ULLONG_WIDTH.

This adds an implementation-defined macro named __BITINT_MAXWIDTH__ to specify that value, which is used by limits.h for the standard macro. This gives us a migration path in the future should a target ever need to support something different than llvm::IntegerType::MAX_INT_BITS.

Diff Detail

Event Timeline

aaron.ballman created this revision.Jan 13 2022, 10:38 AM
aaron.ballman requested review of this revision.Jan 13 2022, 10:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2022, 10:38 AM
Herald added a subscriber: aheejin. · View Herald Transcript
erichkeane accepted this revision.Jan 13 2022, 10:41 AM

CFE changes LGTM, the limits.h/.cpp changes LOOK right, but please give others a chance to take a look.

clang/lib/Sema/SemaType.cpp
2270

Thanks for this!

This revision is now accepted and ready to land.Jan 13 2022, 10:41 AM
erichkeane added inline comments.Jan 13 2022, 10:44 AM
clang/test/Preprocessor/init.c
1523

Actually.... why are we testing WEBASSEMBLY/AArch64 only? I get that these two both do a 'next', but I would presume we'd want a similar test that ends up being for ALL platforms. Its unfortunate the way that this test is setup, but could we perhaps have a 'triple-less' (or a test that just has a massive number of triples?) test of some sort that just validates this value?

aaron.ballman added inline comments.Jan 13 2022, 10:48 AM
clang/test/Preprocessor/init.c
1523

I agree that'd be nice, but it seems pretty orthogonal to this patch too. The "init" tests definitely need some love because they're incredibly onerous. But I'd prefer that be done another time.

As for why only here -- the value is identical for all targets currently, so adding tests for other targets would be a great idea, but not really test much of value. However, I can add the lines to the other targets easily enough if you think there's value.

erichkeane added inline comments.Jan 13 2022, 10:50 AM
clang/test/Preprocessor/init.c
1523

Sure, yeah, I see the lack of value. I guess if anyone ever tries to make it target-specific we can make them do the additional testing instead.

Ping @jyknight or @rsmith for any final thoughts or concerns?

Updated to allow targets to specify the max bitwidth. I was aware that the x86 backend had issues with doing division on larger _BitInt objects, but it turns out *all* backends fail to support _BitInt(129) or wider division. Because division is a pretty common operation on numeric data types, it seemed to be more user-friendly to specify that we only support 128 bits or less. I tried to make it exceptionally clear that I consider this to be a backend bug though -- we will support wider bit widths in the future once backends have been fixed. These changes make that future path easier to opt into.

erichkeane added inline comments.Jan 25 2022, 12:12 PM
clang/lib/Frontend/InitPreprocessor.cpp
918

Could you do an assert that TI.getMaxBitIntWidth() is not greater than the LLVM maximum (just somewhere, here seemed to make the most sense).

Updated based on review feedback. I added an additional assert to test that the value picked by the target meets the requirements from the C standard. (We already had a test case for it in lit, but no reason not to assert for even earlier notification of an issue.)

erichkeane accepted this revision.Jan 25 2022, 1:00 PM

Accept again, thanks! Note the purpose of the assert is to make sure that a backend doesn't make a bad decision, and the test for 'max size' is only validating 1 backend (AND has now been changed to >128 I think).

Ping @jyknight -- I'd like to get this in before the Clang 14 branch, if possible.

jyknight accepted this revision.Jan 28 2022, 11:36 AM

No additional comments, thanks!

Thank you for the reviews! I've commit in 86797fdb6f51d32f285e48b6d3e0fc5b8b852734.

@aaron.ballman I believe this change broke the build starting with:

https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/26915/

/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: error: use of overloaded operator '<<' is ambiguous (with operand types 'const clang::StreamingDiagnostic' and 'typename remove_reference<unsigned long &>::type' (aka 'unsigned long'))
...
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/clang/lib/Sema/SemaType.cpp:2274:23: note: in instantiation of function template specialization 'clang::Sema::SemaDiagnosticBuilder::operator<<<unsigned long, void>' requested here
        << IsUnsigned << TI.getMaxBitIntWidth();
rastogishubham added inline comments.
clang/lib/Sema/SemaType.cpp
2271–2274

Seems like this change broke the greendragon

https://green.lab.llvm.org/green/job/lldb-cmake/40733/console

Can you please take a look and either fix or revert your change?

Error message:

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: error: use of overloaded operator '<<' is ambiguous (with operand types 'const clang::StreamingDiagnostic' and 'typename remove_reference<unsigned long &>::type' (aka 'unsigned long'))

DB << std::move(V);
~~ ^  ~~~~~~~~~~~~

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Sema/Sema.h:1686:16: note: in instantiation of function template specialization 'clang::DiagnosticBuilder::operator<<<unsigned long, void>' requested here

BaseDiag << std::move(V);
         ^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Sema/Sema.h:1760:24: note: in instantiation of function template specialization 'clang::Sema::ImmediateDiagBuilder::operator<<<unsigned long, void>' requested here

*ImmediateDiag << std::move(V);
               ^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/SemaType.cpp:2274:23: note: in instantiation of function template specialization 'clang::Sema::SemaDiagnosticBuilder::operator<<<unsigned long, void>' requested here

<< IsUnsigned << TI.getMaxBitIntWidth();
              ^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1399:35: note: candidate function
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,

^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1405:35: note: candidate function
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,

^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1421:35: note: candidate function
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,

^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1427:35: note: candidate function
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,

^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(int, unsigned long)

DB << std::move(V);
   ^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(int, int)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(int, long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(int, long long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(int, int128)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(int, unsigned int)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(int, unsigned long long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(int, unsigned
int128)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(long, unsigned long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(long long, unsigned long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(int128, unsigned long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(unsigned int, unsigned long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(unsigned long, unsigned long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(unsigned long long, unsigned long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8: note: built-in candidate operator<<(unsigned
int128, unsigned long)
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/SemaType.cpp:13:
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/TypeLocBuilder.h:17:
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/AST/ASTContext.h:19:
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/AST/CanonicalType.h:17:
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/AST/Type.h:29:
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: error: use of overloaded operator '<<' is ambiguous (with operand types 'const clang::StreamingDiagnostic' and 'typename remove_reference<unsigned long &>::type' (aka 'unsigned long'))

DB << std::move(V);
~~ ^  ~~~~~~~~~~~~

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Sema/Sema.h:1762:58: note: in instantiation of function template specialization 'clang::PartialDiagnostic::operator<<<unsigned long, void>' requested here

S.DeviceDeferredDiags[Fn][*PartialDiagId].second << std::move(V);
                                                 ^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/SemaType.cpp:2274:23: note: in instantiation of function template specialization 'clang::Sema::SemaDiagnosticBuilder::operator<<<unsigned long, void>' requested here

<< IsUnsigned << TI.getMaxBitIntWidth();
              ^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1399:35: note: candidate function
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,

^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1405:35: note: candidate function
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,

^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1421:35: note: candidate function
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,

^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/Diagnostic.h:1427:35: note: candidate function
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,

^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(int, unsigned long)

DB << std::move(V);
   ^

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(int, int)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(int, long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(int, long long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(int, int128)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(int, unsigned int)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(int, unsigned long long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(int, unsigned
int128)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(long, unsigned long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(long long, unsigned long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(int128, unsigned long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(unsigned int, unsigned long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(unsigned long, unsigned long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(unsigned long long, unsigned long)
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/include/clang/Basic/PartialDiagnostic.h:71:8: note: built-in candidate operator<<(unsigned
int128, unsigned long)
2 errors generated.

This revision was landed with ongoing or failed builds.Jan 28 2022, 3:19 PM

I verified locally that reverting this patch fixes the build.
Reverted in fad7e491a0770ac4336934030ac67d77e7af5520 to unblock Green Dragon, etc.
@aaron.ballman Please take a look when you get a chance.

I verified locally that reverting this patch fixes the build.
Reverted in fad7e491a0770ac4336934030ac67d77e7af5520 to unblock Green Dragon, etc.
@aaron.ballman Please take a look when you get a chance.

Thank you for the revert and sorry for not noticing the breakage sooner! I've corrected the issue in a6cabd98021fe357d666614601eca2031bf775de and re-committed. I'll keep an eye on the bots this morning to see if there's any further fallout.

mariospr added a subscriber: mariospr.EditedFeb 22 2022, 10:06 AM

Updated to allow targets to specify the max bitwidth. I was aware that the x86 backend had issues with doing division on larger _BitInt objects, but it turns out *all* backends fail to support _BitInt(129) or wider division. Because division is a pretty common operation on numeric data types, it seemed to be more user-friendly to specify that we only support 128 bits or less. I tried to make it exceptionally clear that I consider this to be a backend bug though -- we will support wider bit widths in the future once backends have been fixed. These changes make that future path easier to opt into.

Hi! I know this question might be very hard to answer but even so I hope you don't mind me trying 😇 ... do you know whether there is any estimation of when _BitInt(N) / N > 128 will be supported once again in Clang? I'm not an expert at all on this topic (just registered here to comment! 🙂 ) but this hit us in a Chromium-based project which relied on _BITINT(256) now that Chromium 100 updated to Clang 15. And while we're already looking into alternative ways of dealing with is (e.g. maybe using Boost's multiprecision module), it would be very interesting if we could get a sense (if it's even possible) on how long we could expect the current situation to last.

Again, I understand there might not be a clear answer for this at the moment but, since I'm no expert on the matter and I couldn't figure out myself the exact situation by reading this and other tickets in the tracker, I thought I'd ask just in case.

Thanks in advance regardless of the answer!
Mario

Updated to allow targets to specify the max bitwidth. I was aware that the x86 backend had issues with doing division on larger _BitInt objects, but it turns out *all* backends fail to support _BitInt(129) or wider division. Because division is a pretty common operation on numeric data types, it seemed to be more user-friendly to specify that we only support 128 bits or less. I tried to make it exceptionally clear that I consider this to be a backend bug though -- we will support wider bit widths in the future once backends have been fixed. These changes make that future path easier to opt into.

Hi! I know this question might be very hard to answer but even so I hope you don't mind me trying 😇 ... do you know whether there is any estimation of when _BitInt(N) / N > 128 will be supported once again in Clang? I'm not an expert at all on this topic (just registered here to comment! 🙂 ) but this hit us in a Chromium-based project which relied on _BITINT(256) now that Chromium 100 updated to Clang 15. And while we're already looking into alternative ways of dealing with is (e.g. maybe using Boost's multiprecision module), it would be very interesting if we could get a sense (if it's even possible) on how long we could expect the current situation to last.

Again, I understand there might not be a clear answer for this at the moment but, since I'm no expert on the matter and I couldn't figure out myself the exact situation by reading this and other tickets in the tracker I thought, I thought I'd ask just in case.

Thanks in advance regardless of the answer!
Mario

Hi Mario! Thank you for inquiring! As you may know, we disabled _BitInt this large since we couldn't handle division/modulus in the code generators. There is an RFC currently underway (as of today, we have the patches to start supporting this in the backend!) here: https://discourse.llvm.org/t/rfc-add-support-for-division-of-large-bitint-builtins-selectiondag-globalisel-clang/60329

I'm hopeful that the answer here is 'in the near future'. @mgehre-amd should be able to at least partially revert this patch once the library changes/llvm code generator changes make it in.

Updated to allow targets to specify the max bitwidth. I was aware that the x86 backend had issues with doing division on larger _BitInt objects, but it turns out *all* backends fail to support _BitInt(129) or wider division. Because division is a pretty common operation on numeric data types, it seemed to be more user-friendly to specify that we only support 128 bits or less. I tried to make it exceptionally clear that I consider this to be a backend bug though -- we will support wider bit widths in the future once backends have been fixed. These changes make that future path easier to opt into.

Hi! I know this question might be very hard to answer but even so I hope you don't mind me trying 😇 ... do you know whether there is any estimation of when _BitInt(N) / N > 128 will be supported once again in Clang? I'm not an expert at all on this topic (just registered here to comment! 🙂 ) but this hit us in a Chromium-based project which relied on _BITINT(256) now that Chromium 100 updated to Clang 15. And while we're already looking into alternative ways of dealing with is (e.g. maybe using Boost's multiprecision module), it would be very interesting if we could get a sense (if it's even possible) on how long we could expect the current situation to last.

Again, I understand there might not be a clear answer for this at the moment but, since I'm no expert on the matter and I couldn't figure out myself the exact situation by reading this and other tickets in the tracker I thought, I thought I'd ask just in case.

Thanks in advance regardless of the answer!
Mario

Hi Mario! Thank you for inquiring! As you may know, we disabled _BitInt this large since we couldn't handle division/modulus in the code generators. There is an RFC currently underway (as of today, we have the patches to start supporting this in the backend!) here: https://discourse.llvm.org/t/rfc-add-support-for-division-of-large-bitint-builtins-selectiondag-globalisel-clang/60329

I'm hopeful that the answer here is 'in the near future'. @mgehre-amd should be able to at least partially revert this patch once the library changes/llvm code generator changes make it in.

I agree with what Erich is saying here. My intention is to increase back to the LLVM maximum again once the IR we generate stops crashing LLVM. However, there's likely to be performance issues with "very large" bit-precise integer types until we've gone through the backend and optimized for those cases (both compile time and runtime performance concerns), so we might want to bump up to something lower than the theoretical max for a while still (though hopefully usefully bigger than 128!).

Hi Mario! Thank you for inquiring! As you may know, we disabled _BitInt this large since we couldn't handle division/modulus in the code generators. There is an RFC currently underway (as of today, we have the patches to start supporting this in the backend!) here: https://discourse.llvm.org/t/rfc-add-support-for-division-of-large-bitint-builtins-selectiondag-globalisel-clang/60329

I'm hopeful that the answer here is 'in the near future'. @mgehre-amd should be able to at least partially revert this patch once the library changes/llvm code generator changes make it in.

I agree with what Erich is saying here. My intention is to increase back to the LLVM maximum again once the IR we generate stops crashing LLVM. However, there's likely to be performance issues with "very large" bit-precise integer types until we've gone through the backend and optimized for those cases (both compile time and runtime performance concerns), so we might want to bump up to something lower than the theoretical max for a while still (though hopefully usefully bigger than 128!).

Wow, this is some seriously quick response time, kudos for that!

Also, thanks a lot for the clarification: it's more clear to me now what the situation actually is, and I'm happy to see there's already an ongoing plan to get this fixed, even if it will take some time. It was not clear to me when I read this ticket the first time whether there was some active work going on already, so thanks a lot for the pointers, that makes things more clear.

Again, thanks a lot for your help, much appreciated! I'll keep an eye on this ticket and the RFC to be aware when things change.