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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
