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

Tue Mar 22 09:39:38 PDT 2022

erichkeane added inline comments.

================
Comment at: clang/lib/Lex/LiteralSupport.cpp:1232

+  if (llvm::isPowerOf2_32(radix)) {
+    unsigned BitsPerDigit = llvm::Log2(radix);
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122234