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