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: > > > 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? > I was hoping to avoid something like that, but I think it's not entirely unreasonable as a stopgap measure either. How about we name it `-fexperimental-max-bitint-width=N` to make it clear that this option is not expected to stick around forever? (I'd like to not ship a release of Clang with this flag). I suppose we'd still have to make sure the user doesn't specify a value larger than what LLVM supports, too. 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
