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

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd marked 2 inline comments as done.
mgehre-amd added inline comments.
Herald added a subscriber: MaskRay.


================
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:
> > 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.
Ok, then let me propose the following:
- Keep the default max. _BitInt size limit at 128 with this PR
- Remove the code that emits the float-to-int/int-to-float warnings for bit _BitInts
- Add a cc1 option -fmax-bitint-size=N to allow overwriting the max. _BitInt size for tests (so we can test big division even when float-to-int isn't there yet)

What do you think?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122234