[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size
mgehre-amd added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:315 #define BENIGN_ENUM_LANGOPT(Name, Type, Bits, Default, Description) -#define BENIGN_VALUE_LANGOPT(Name, Type, Bits, Default, Description) +#define BENIGN_VALUE_LANGOPT(Name, Bits, Default, Description) #include "clang/Basic/LangOptions.def" This macro was taking the wrong number of arguments, but nobody noticed because there was no `BENIGN_VALUE_LANGOPT` before. 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
[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size
mgehre-amd updated this revision to Diff 418861.
mgehre-amd added a comment.
Herald added a subscriber: dexonsmith.
- Remove diagnostics for float-to-bitint
- Remove lexer changes (will be another PR)
- Add -fexperimental-max-bitint-width=N
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122234/new/
https://reviews.llvm.org/D122234
Files:
clang/include/clang/Basic/LangOptions.def
clang/include/clang/Basic/TargetInfo.h
clang/include/clang/Driver/Options.td
clang/lib/Basic/TargetInfo.cpp
clang/lib/Driver/ToolChains/CommonArgs.cpp
clang/lib/Driver/ToolChains/CommonArgs.h
clang/lib/Driver/ToolChains/Gnu.cpp
clang/lib/Driver/ToolChains/Hexagon.cpp
clang/lib/Driver/ToolChains/MinGW.cpp
clang/lib/Serialization/ASTReader.cpp
clang/test/Driver/linux-ld.c
clang/test/Sema/large-bit-int.c
Index: clang/test/Sema/large-bit-int.c
===
--- /dev/null
+++ clang/test/Sema/large-bit-int.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fexperimental-max-bitint-width=1024 -fsyntax-only -verify %s -Wno-unused -triple x86_64-gnu-linux
+
+void f() {
+ _Static_assert(__BITINT_MAXWIDTH__ == 1024, "Macro value is unexpected.");
+
+ _BitInt(1024) a;
+ unsigned _BitInt(1024) b;
+
+ _BitInt(8388609) c;// expected-error {{signed _BitInt of bit sizes greater than 1024 not supported}}
+ unsigned _BitInt(0xFF) d; // expected-error {{unsigned _BitInt of bit sizes greater than 1024 not supported}}
+}
Index: clang/test/Driver/linux-ld.c
===
--- clang/test/Driver/linux-ld.c
+++ clang/test/Driver/linux-ld.c
@@ -174,7 +174,7 @@
// RUN: --sysroot=%S/Inputs/basic_linux_tree \
// RUN: | FileCheck --check-prefix=CHECK-CLANG-NO-LIBGCC-STATIC %s
// CHECK-CLANG-NO-LIBGCC-STATIC: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
-// CHECK-CLANG-NO-LIBGCC-STATIC: "--start-group" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
+// CHECK-CLANG-NO-LIBGCC-STATIC: "--start-group" "{{[^"]*}}/libclang_rt.bitint{{[^"]*}}" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
//
// RUN: %clang -static-pie -no-canonical-prefixes %s -### -o %t.o 2>&1 \
// RUN: --target=x86_64-unknown-linux -rtlib=platform --unwindlib=platform \
@@ -189,7 +189,7 @@
// CHECK-CLANG-LD-STATIC-PIE: "text"
// CHECK-CLANG-LD-STATIC-PIE: "-m" "elf_x86_64"
// CHECK-CLANG-LD-STATIC-PIE: "{{.*}}rcrt1.o"
-// CHECK-CLANG-LD-STATIC-PIE: "--start-group" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
+// CHECK-CLANG-LD-STATIC-PIE: "--start-group" "{{[^"]*}}/libclang_rt.bitint{{[^"]*}}" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
//
// RUN: %clang -static-pie -pie -no-canonical-prefixes %s -no-pie -### -o %t.o 2>&1 \
// RUN: --target=x86_64-unknown-linux -rtlib=platform --unwindlib=platform \
@@ -204,7 +204,7 @@
// CHECK-CLANG-LD-STATIC-PIE-PIE: "text"
// CHECK-CLANG-LD-STATIC-PIE-PIE: "-m" "elf_x86_64"
// CHECK-CLANG-LD-STATIC-PIE-PIE: "{{.*}}rcrt1.o"
-// CHECK-CLANG-LD-STATIC-PIE-PIE: "--start-group" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
+// CHECK-CLANG-LD-STATIC-PIE-PIE: "--start-group" "{{[^"]*}}/libclang_rt.bitint{{[^"]*}}" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
//
// RUN: %clang -static-pie -static -no-canonical-prefixes %s -no-pie -### -o %t.o 2>&1 \
// RUN: --target=x86_64-unknown-linux -rtlib=platform --unwindlib=platform \
@@ -219,7 +219,7 @@
// CHECK-CLANG-LD-STATIC-PIE-STATIC: "text"
// CHECK-CLANG-LD-STATIC-PIE-STATIC: "-m" "elf_x86_64"
// CHECK-CLANG-LD-STATIC-PIE-STATIC: "{{.*}}rcrt1.o"
-// CHECK-CLANG-LD-STATIC-PIE-STATIC: "--start-group" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
+// CHECK-CLANG-LD-STATIC-PIE-STATIC: "--start-group" "{{[^"]*}}/libclang_rt.bitint{{[^"]*}}" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
//
// RUN: %clang -static-pie -nopie -no-canonical-prefixes %s -no-pie -### -o %t.o 2>&1 \
// RUN: --target=x86_64-unknown-linux -rtlib=platform \
@@ -318,7 +318,7 @@
// CHECK-LD-64-STATIC: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/10.2.0/../../../../x86_64-unknown-linux/lib"
// CHECK-LD-64-STATIC: "-L[[SYSROOT]]/lib"
// CHECK-LD-64-STATIC: "-L[[SYSROOT]]/usr/lib"
-// CHECK-LD-64-STATIC: "--start-group" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
+// CHECK-LD-64-STATIC: "--start-group" "{{[^"]*}}/libclang_rt.bitint{{[^"]*}}" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
// RUN: %clang -no-pie -### %s --target=x86_64-unknown-linux -rtlib=platform --unwindlib=platform -shared -static \
// RUN: --gcc-toolchain= --sysroot=%S/Inputs/basic_linux_tree 2>&1 | FileCheck --check-prefix=CHECK-LD-SHARED-STATIC %s
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -312,7 +312,7 @@
#define BENIGN_LANGOPT(Name, Bits, Default, Description)
#define BENIGN_ENUM_LANGOPT(Name, Type, Bits, Default, Description)
[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size
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
[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size
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
[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size
mgehre-amd marked 3 inline comments as done.
mgehre-amd 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">;
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.
Comment at: clang/lib/Lex/LiteralSupport.cpp:1232
+ if (llvm::isPowerOf2_32(radix)) {
+unsigned BitsPerDigit = llvm::Log2(radix);
erichkeane wrote:
> mgehre-amd wrote:
> > erichkeane wrote:
> > > Can you explain what is going on here? This isn't at all obvious to me.
> > 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.
> 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.
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.
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
[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size
erichkeane added inline comments.
Comment at: clang/lib/Lex/LiteralSupport.cpp:1232
+ if (llvm::isPowerOf2_32(radix)) {
+unsigned BitsPerDigit = llvm::Log2(radix);
mgehre-amd wrote:
> erichkeane wrote:
> > Can you explain what is going on here? This isn't at all obvious to me.
> 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.
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.
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
[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size
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: > 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 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
[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size
mgehre-amd marked an inline comment as done.
mgehre-amd 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">;
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.
Comment at: clang/lib/Lex/LiteralSupport.cpp:1232
+ if (llvm::isPowerOf2_32(radix)) {
+unsigned BitsPerDigit = llvm::Log2(radix);
erichkeane wrote:
> Can you explain what is going on here? This isn't at all obvious to me.
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.
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
[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size
erichkeane 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">;
Can you explain the issue here? This is supposed to be well-defined behavior.
Comment at: clang/include/clang/Basic/TargetInfo.h:599
virtual size_t getMaxBitIntWidth() const {
-// FIXME: this value should be llvm::IntegerType::MAX_INT_BITS, which is
-// maximum bit width that LLVM claims its IR can support. However, most
-// backends currently have a bug where they only support division
-// operations on types that are <= 128 bits and crash otherwise. We're
-// setting the max supported value to 128 to be conservative.
-return 128;
+// TODO: Return 128 for targets that don't link libbitint?
+return llvm::IntegerType::MAX_INT_BITS;
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'.
Comment at: clang/lib/Lex/LiteralSupport.cpp:1232
+ if (llvm::isPowerOf2_32(radix)) {
+unsigned BitsPerDigit = llvm::Log2(radix);
Can you explain what is going on here? This isn't at all obvious to me.
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
[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size
mgehre-amd created this revision.
mgehre-amd added reviewers: aaron.ballman, erichkeane.
Herald added subscribers: luke957, s.egerton, mstorsjo, simoncook,
fedor.sergeev, dschuff.
Herald added a project: All.
mgehre-amd requested review of this revision.
Herald added subscribers: pcwang-thead, aheejin.
Herald added a project: clang.
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
- clang/Basic/TargetInfo: Increase getMaxBitIntWidth to
llvm::IntegerType::MAX_INT_BITS
- Sema: Diagnose when converting _BitInt to/from floating point for bit width >
128 because that breaks in the backend
- Lex: Speedup parsing of large integer literals with a power-of-two radix, so
parsing a hex literal with 2097152 digits (=MAX_INT_BITS) doesn't take forever
[0]
https://discourse.llvm.org/t/rfc-add-support-for-division-of-large-bitint-builtins-selectiondag-globalisel-clang/60329
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D122234
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Basic/TargetInfo.h
clang/include/clang/Sema/Sema.h
clang/lib/Driver/ToolChains/CommonArgs.cpp
clang/lib/Driver/ToolChains/CommonArgs.h
clang/lib/Driver/ToolChains/Gnu.cpp
clang/lib/Driver/ToolChains/MinGW.cpp
clang/lib/Lex/LiteralSupport.cpp
clang/lib/Sema/Sema.cpp
clang/lib/Sema/SemaCast.cpp
clang/test/CodeGen/ext-int-cc.c
clang/test/Driver/linux-ld.c
clang/test/Lexer/bitint-constants.c
clang/test/Preprocessor/init-aarch64.c
clang/test/Preprocessor/init.c
clang/test/Sema/ext-int.c
clang/test/SemaCXX/ext-int.cpp
Index: clang/test/SemaCXX/ext-int.cpp
===
--- clang/test/SemaCXX/ext-int.cpp
+++ clang/test/SemaCXX/ext-int.cpp
@@ -17,7 +17,7 @@
unsigned _BitInt(5) e = 5;
_BitInt(5) unsigned f;
- _BitInt(-3) g; // expected-error{{signed _BitInt of bit sizes greater than 128 not supported}}
+ _BitInt(-3) g; // expected-error{{signed _BitInt of bit sizes greater than 8388608 not supported}}
_BitInt(0) h; // expected-error{{signed _BitInt must have a bit size of at least 2}}
_BitInt(1) i; // expected-error{{signed _BitInt must have a bit size of at least 2}}
_BitInt(2) j;;
@@ -29,12 +29,12 @@
constexpr _BitInt(7) o = 33;
// Check imposed max size.
- _BitInt(129) p; // expected-error {{signed _BitInt of bit sizes greater than 128 not supported}}
- unsigned _BitInt(0xFF) q; // expected-error {{unsigned _BitInt of bit sizes greater than 128 not supported}}
+ _BitInt(8388609) p;// expected-error {{signed _BitInt of bit sizes greater than 8388608 not supported}}
+ unsigned _BitInt(0xFF) q; // expected-error {{unsigned _BitInt of bit sizes greater than 8388608 not supported}}
// Ensure template params are instantiated correctly.
- // expected-error@5{{signed _BitInt of bit sizes greater than 128 not supported}}
- // expected-error@6{{unsigned _BitInt of bit sizes greater than 128 not supported}}
+ // expected-error@5{{signed _BitInt of bit sizes greater than 8388608 not supported}}
+ // expected-error@6{{unsigned _BitInt of bit sizes greater than 8388608 not supported}}
// expected-note@+1{{in instantiation of template class }}
HasExtInt<-1> r;
// expected-error@5{{signed _BitInt must have a bit size of at least 2}}
@@ -285,3 +285,16 @@
void FromPaper2(_BitInt(8) a1, _BitInt(24) a2) {
static_assert(is_same::value, "");
}
+
+void LargeBitIntToFloat(_BitInt(129) a) {
+ float fa = a; // expected-error {{cannot convert '_BitInt' operands of more than 128 bits to floating point}}
+ fa = static_cast(a); // expected-error {{cannot convert '_BitInt' operands of more than 128 bits to floating point}}
+ fa = a + 0.1; // expected-error {{cannot convert '_BitInt' operands of more than 128 bits to floating point}}
+}
+
+_BitInt(129) LargeBitIntFromFloat(float f) {
+ _BitInt(129) a = f; // expected-error {{cannot convert floating point operands to a '_BitInt' of more than 128 bits}}
+ a = f; // expected-error {{cannot convert floating point operands to a '_BitInt' of more than 128 bits}}
+ a = static_cast<_BitInt(129)>(f); // expected-error {{cannot convert floating point operands to a '_BitInt' of more than 128 bits}}
+ return f; // expected-error {{cannot convert floating point operands to a '_BitInt' of more than 128 bits}}
+}
Index: clang/test/Sema/ext-int.c
===
--- clang/test/Sema/ext-int.c
+++ clang/test/Sema/ext-int.c
@@ -73,3 +73,16 @@
void FromPaper2(_BitInt(8) a1, _BitInt(24) a2) {
_Static_assert(EXPR_HAS_TYPE(a1 * (_BitInt(32))a2, _BitInt(32)), "");
}
+
+
+void LargeBitIntToFloat(_BitInt(129) a) {
+ float fa = a; // expected-error {{cannot convert '_BitInt' operands of more than 128 bits to floating point}}
