Page MenuHomePhabricator

[clang] Link libbitint for large division of _BitInt
AbandonedPublic

Authored by mgehre-amd on Mar 22 2022, 8:24 AM.

Details

Summary

According to the RFC [0], this review contains the clang parts of large integer divison for _BitInt.

It contains the following parts:

  • Driver: Gnu, MinGW: Link libbitint when available
  • Add -fexperimental_max_bitint_width=N

[0] https://discourse.llvm.org/t/rfc-add-support-for-division-of-large-bitint-builtins-selectiondag-globalisel-clang/60329

Diff Detail

Event Timeline

mgehre-amd created this revision.Mar 22 2022, 8:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2022, 8:24 AM
mgehre-amd requested review of this revision.Mar 22 2022, 8:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2022, 8:24 AM
erichkeane added inline comments.Mar 22 2022, 8:31 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8422 ↗(On Diff #417304)

Can you explain the issue here? This is supposed to be well-defined behavior.

clang/include/clang/Basic/TargetInfo.h
612

This is definitely a TODO we need to do before accepting this. The purpose of this function is that each individual target can 'override' this function. This is an 'opt in'.

clang/lib/Lex/LiteralSupport.cpp
1232 ↗(On Diff #417304)

Can you explain what is going on here? This isn't at all obvious to me.

mgehre-amd marked an inline comment as done.Mar 22 2022, 9:28 AM
mgehre-amd added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
8422 ↗(On Diff #417304)

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.

clang/lib/Lex/LiteralSupport.cpp
1232 ↗(On Diff #417304)

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.

mgehre-amd marked an inline comment as done.Mar 22 2022, 9:38 AM
aaron.ballman added inline comments.Mar 22 2022, 9:38 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8422 ↗(On Diff #417304)

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

erichkeane added inline comments.Mar 22 2022, 9:39 AM
clang/lib/Lex/LiteralSupport.cpp
1232 ↗(On Diff #417304)

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.

mgehre-amd marked 3 inline comments as done.Mar 22 2022, 9:53 AM
mgehre-amd added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
8422 ↗(On Diff #417304)

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.

clang/lib/Lex/LiteralSupport.cpp
1232 ↗(On Diff #417304)

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.

aaron.ballman added inline comments.Mar 22 2022, 10:04 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8422 ↗(On Diff #417304)

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.

mgehre-amd marked 2 inline comments as done.Mar 28 2022, 1:52 AM
mgehre-amd added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
8422 ↗(On Diff #417304)

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?

aaron.ballman added inline comments.Mar 28 2022, 4:33 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8422 ↗(On Diff #417304)

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.

  • Remove diagnostics for float-to-bitint
  • Remove lexer changes (will be another PR)
  • Add -fexperimental-max-bitint-width=N
mgehre-amd edited the summary of this revision. (Show Details)Mar 29 2022, 6:44 AM
mgehre-amd added inline comments.
clang/lib/Serialization/ASTReader.cpp
315

This macro was taking the wrong number of arguments, but nobody noticed because there was no BENIGN_VALUE_LANGOPT before.

mgehre-amd retitled this revision from [clang] Link libbitint for large division of _BitInt; increase max _BitInt size to [clang] Link libbitint for large division of _BitInt.Mar 29 2022, 6:45 AM

This looks generally correct to me, but precommit CI is currently failing with a relevant issue (what a nice change of pace):

Failed Tests (1):

Clang :: Driver/linux-ld.c
clang/include/clang/Basic/TargetInfo.h
603

So we get the return type correct? (I doubt this matters.)

clang/lib/Serialization/ASTReader.cpp
315

Good catch!

MaskRay added inline comments.Apr 11 2022, 12:48 PM
clang/lib/Driver/ToolChains/Hexagon.cpp
456

The runtime library order matters and roughly reflects a topological order.
For instances, -lc is higher level than libgcc_s/libclang_rt.builtins.a therefore -lc precedes them.

mgehre-amd marked an inline comment as done.Jun 8 2022, 3:55 AM

I split the introduction of -fexperimental-max-bitint-width into https://reviews.llvm.org/D127287 because there is still discussion about the bitint libraries in the backend PRs.

mgehre-amd abandoned this revision.Tue, Jul 19, 4:36 AM

Instead of linking to libbitint, I instead created a backend pass to transform div/rem instructions: https://reviews.llvm.org/D130076