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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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">;
----------------
mgehre-amd wrote:
> 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.
> 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.

This is certainly better than crashing, no doubt.

> 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,

That's the problem -- this is a new basic datatype; we can't make assumptions that our constituent uses cases are the common pattern. My experience thus far with this feature has been that people are using it for all sorts of reasons in ways I wouldn't have expected, like as a big int, as a regular int that doesn't integer promote, as bit-fields, etc.

To me, the bar for whether we allow large bit widths than we do today is: do all basic mathematical operators on any bit-width work correctly at runtime without crashing or major compile-time or runtime performance issues for the given target you want to change the limit for? Assignment and conversion are basic mathematical operators I'd definitely expect to work; these aren't weird situations -- assigning a float to an integer happens with some regularity, so there's no reason to assume it won't happen when the integer is a larger bit-precise integer: https://godbolt.org/z/hx5sYbjGK.

I'd *much* rather slow-roll the feature than have people's first impression be that they can't trust the feature because it's too flaky. The whole point to the `BITINT_MAXWIDTH` macro is to give the user a reliable way to know what bit widths are supported; lying will not do us any favors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122234