[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd marked 3 inline comments as done.
mgehre-amd added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8422
 
+def err_int_to_float_bit_int_max_size : Error<
+  "cannot convert '_BitInt' operands of more than %0 bits to floating point">;
----------------
aaron.ballman wrote:
> mgehre-amd wrote:
> > erichkeane wrote:
> > > Can you explain the issue here?  This is supposed to be well-defined behavior.
> > I saw `Assertion LC != RTLIB::UNKNOWN_LIBCALL && "Don't know how to expand this SINT_TO_FP!"' failed.` in the backend. I think we would need to add library calls for floating to bitint (and vice versa) to the bitint library to enable that.
> Good catch! I think we'll need to solve that before we can enable wide bit-width support (users are going to expect assignment and initialization to not crash as those are basic builtin operators). FWIW, this is a reproducer from Clang 13 where we still allowed large widths: https://godbolt.org/z/hvzWq1KTK
I don't think I agree. 

Right now users will get a diagnostic that _BitInt > 128 bit cannot be used. With this PR, they can use them but get a diagnostic when they try to convert large _BitInts to floats or back.
I think there is huge value in allowing large _BitInts even when float to/from conversion will cause a clang diagnostic because at least in my use case that is pretty uncommon,
so I propose to move forward with this PR (lifting the limit on _BitInt width) without having the compiler-rt support for float conversions.


================
Comment at: clang/lib/Lex/LiteralSupport.cpp:1232
 
+  if (llvm::isPowerOf2_32(radix)) {
+    unsigned BitsPerDigit = llvm::Log2(radix);
----------------
erichkeane wrote:
> mgehre-amd wrote:
> > erichkeane wrote:
> > > Can you explain what is going on here?  This isn't at all obvious to me.
> > When I adapted the test case  clang/test/Lexer/bitint-constants.c https://reviews.llvm.org/D122234#change-GjEFAAe2jzfe to test literals with 2097152 hex digits, clang spend minutes in the code below to try to turn that literal into a ApInt of 8388608 bits.
> > 
> > That is because the code below does a generic `Val *= Radix` and `Val += Digit`. The `Val *= Radix` is very slow on a ApInt with 8388608 bits, and it's also not needed when
> > we know that `Radix` is a power of two. Then one can just copy the bits from each Digit into the right position in Val.
> > 
> > If the integer literals can just be 128 bits or something, it doesn't matter, but now we need to support really big integer literals with having increased the limit on _BitInt width.
> Hmm, so this is only really an issue with base-10 integer values?  Is the same 'clang spent minutes in...' still a problem for base 10 literals?  It would be unfortunate if we cannot just fix that.
> 
> Additionally, I'd like some comment to explain this, it isn't particularly clear what it is doing.
Yes, clang is still slow if a user writes a decimal literal to initialize a 8388608  bit ApInt, but maybe that is just what the called for.
You can also have a very slow compilation when you create complicated recursive templates or huge macros; that isn't inherently the compilers fault.

I mainly fixed it for the hex case to be able to write a test case which says `this literal is still accepted` and `this literal is too big and cannot be represented`.

I will add comments to make the code clearer.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122234/new/

https://reviews.llvm.org/D122234