[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
https://github.com/hvdijk closed https://github.com/llvm/llvm-project/pull/81175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
hvdijk wrote: > I would really like to avoid Clang 18 being released in this broken state, if > possible, and I see no way short of a revert to realistically achieve that. It's too late for that now, it's out there, and it's now everybody else's problem regardless of what Clang does in the future. Because of that, this PR no longer serves its purpose. I cannot imagine that even a single user will be happy with this, but this is what you decided. So be it. https://github.com/llvm/llvm-project/pull/81175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
hvdijk wrote: That would be nice, but that will take time, and I did wait months already before creating this PR. Leaving Clang in its current state until someone steps up to fix it, in my opinion, does a massive disservice to users. If code is written to use `_BitInt`, and it correctly uses configure or CMake checks to try to detect compiler support, and fall back to some other implementation if `_BitInt` is unavailable, such code would, with Clang, use the non-working implementation. In which case the configure or CMake check would end up amended to "and if Clang, don't bother". From experience, such checks remain for far too long after the original problem is fixed; leaving Clang in its current state, I suspect, ensures that once Clang behaves correctly, still `_BitInt` will not be used. I'm happy if someone is willing to step up to fix the implementation, but I am not able to do it, and I would really like to avoid Clang 18 being released in this broken state, if possible, and I see no way short of a revert to realistically achieve that. https://github.com/llvm/llvm-project/pull/81175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
rjmccall wrote: There's no such thing as "this is impossible to do". Clang, as the frontend, is responsible for emitting IR that has gets the effect we want. If that means contorting the IR we generate to do ugly things like memcpying into a temporary, that's our life. I am not surprised that we do not get any sort of reliable behavior from backends for `iNNN` when that's not architectural-legal. I'm pretty sure I raised that concern during the review. https://github.com/llvm/llvm-project/pull/81175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
hvdijk wrote: > At least that shouldn't be a problem: LLVM has separate concepts of "store > size" and "alloc size" where only the latter rounds up to alignment. So `load > i129` is specified to access only 9 bytes, not 16 bytes. Sure, but when it appears inside a struct, the memory reserved is based on the alloc size, not the store size, see `StructLayout`. That applies even for a packed struct. https://github.com/llvm/llvm-project/pull/81175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
nikic wrote: > > It should be technically possible for Clang to give _BitInt(N) an alignment > > that is independent from LLVM's alignment > > I'm not sure it's even technically possible: if loading a `_BitInt(129)` from > memory should load 3 bytes, but it is translated to an LLVM IR load of `i129` > that loads 4 bytes, then even if the last byte is ignored, simply attempting > to load it may access memory out of bounds and crash the program. Storing > `i129`, it would clobber the next byte of memory. At least that shouldn't be a problem: LLVM has separate concepts of "store size" and "alloc size" where only the latter rounds up to alignment. So `load i129` is specified to access only 9 bytes, not 16 bytes. https://github.com/llvm/llvm-project/pull/81175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
hvdijk wrote: > It should be technically possible for Clang to give _BitInt(N) an alignment > that is independent from LLVM's alignment I'm not sure it's even technically possible: if loading a `_BitInt(129)` from memory should load 3 bytes, but it is translated to an LLVM IR load of `i129` that loads 4 bytes, then even if the last byte is ignored, simply attempting to load it may access memory out of bounds and crash the program. Storing `i129`, it would clobber the next byte of memory. https://github.com/llvm/llvm-project/pull/81175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
nikic wrote: Thanks for providing proper context. The second issue does look pretty serious to me. It's a regression from the i128 alignment changes in LLVM 18. It should be technically possible for Clang to give `_BitInt(N)` an alignment that is independent from LLVM's alignment, but clearly Clang doesn't do everything that's necessary for that now, such as emitting GEPs in canonical i8 form with explicitly LLVM-independent offset calculation. https://github.com/llvm/llvm-project/pull/81175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
https://github.com/nikic dismissed https://github.com/llvm/llvm-project/pull/81175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
hvdijk wrote:
> I find your PR description very vague. Please be more specific about which
> ICEs and miscompilations you are concerned about (godbolt please). Is this
> _just_ about the open alignment question, or also something else?
I linked to where one major example had already been provided, but let me put
it in here as well.
https://godbolt.org/z/4jTrW4fcP
```
clang++: /root/llvm-project/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:759:
void {anonymous}::CGRecordLowering::clipTailPadding(): Assertion
`Prior->FD->hasAttr() && "should not have reused this
field's tail padding"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and
include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0. Program arguments:
/opt/compiler-explorer/clang-assertions-trunk/bin/clang++ -gdwarf-4 -g -o
/app/output.s -mllvm --x86-asm-syntax=intel -S
--gcc-toolchain=/opt/compiler-explorer/gcc-snapshot -fcolor-diagnostics
-fno-crash-diagnostics
1. parser at end of file
2. :9:3: LLVM IR generation of declaration 'g'
3. :9:3: Generating code for declaration 'g'
#0 0x0389ed98 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int)
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x389ed98)
#1 0x0389ca7c llvm::sys::CleanupOnSignal(unsigned long)
(/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x389ca7c)
#2 0x037e4f58 CrashRecoverySignalHandler(int)
CrashRecoveryContext.cpp:0:0
#3 0x7f7cec242520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
#4 0x7f7cec2969fc pthread_kill (/lib/x86_64-linux-gnu/libc.so.6+0x969fc)
#5 0x7f7cec242476 gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x42476)
#6 0x7f7cec2287f3 abort (/lib/x86_64-linux-gnu/libc.so.6+0x287f3)
#7 0x7f7cec22871b (/lib/x86_64-linux-gnu/libc.so.6+0x2871b)
#8 0x7f7cec239e96 (/lib/x86_64-linux-gnu/libc.so.6+0x39e96)
#9 0x03baa784 (anonymous
namespace)::CGRecordLowering::clipTailPadding() CGRecordLayoutBuilder.cpp:0:0
#10 0x03bb0253 (anonymous namespace)::CGRecordLowering::lower(bool)
CGRecordLayoutBuilder.cpp:0:0
```
Here's another:
https://godbolt.org/z/eKahY86Yd
```
_BitInt(129) *f(_BitInt(129) *p) {
return p + 1;
}
char *f(char *p) {
return p + sizeof(_BitInt(129));
}
```
These should obviously produce identical code, but it results in
```
f(_BitInt(129)*):# @f(_BitInt(129)*)
lea rax, [rdi + 32]
ret
f(char*): # @f(char*)
lea rax, [rdi + 24]
ret
```
The codegen is just completely broken, and the fact that it passes Clang's
testing is simply due to the fact that we have close to zero tests that it
works in any capacity.
Note that this revert doesn't remove the support entirely, it only moves it
back behind the same `-fexperimental-max-bitint-width=` option that was already
there before.
https://github.com/llvm/llvm-project/pull/81175
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
https://github.com/nikic requested changes to this pull request. I don't think that this makes sense, given that this has already been allowed in previous stable clang releases. That's not a step you can really take back. > but internal compiler errors and seriously wrong code I find your PR description very vague. Please be more specific about which ICEs and miscompilations you are concerned about (godbolt please). Is this *just* about the open alignment question, or also something else? https://github.com/llvm/llvm-project/pull/81175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
AaronBallman wrote: CC @phoebewang as well https://github.com/llvm/llvm-project/pull/81175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
AaronBallman wrote: I'm not in favor of this revert at the moment, but I'd like to hear more from our ABI folks (@rjmccall @hjl-tools specifically). It does not seem reasonable to me that we basically have no path forward to support `_BitInt` with larger sizes, which is one of the key points to the feature in the first place. Should Clang change the IR we produce for LLVM? Should LLVM change? Should the ABI change? Some combination of these? https://github.com/llvm/llvm-project/pull/81175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Harald van Dijk (hvdijk)
Changes
This reverts commit def720726b73e0d7ab139376ab3ea955f25f4d89.
As noted in #60925 and in D86310, with the current implementation of
`_BitInt` in Clang, we can have either a correct `__int128` implementation, or
a correct `_BitInt(128)` implementation, but not both: having a correct
`__int128` implementation results in a mostly-working `_BitInt(65-128)` type,
but internal compiler errors and seriously wrong code for `_BitInt(129+)`. The
currently specified ABI is not implementable on top of LLVM's integer types,
and attempts to get the ABI changed to something we can implement have resulted
in nothing, so I think the safest thing to do at this time is to make sure
Clang reports this as an error in exactly the same way that it does for all
other architectures. I think it was simply too soon to lift that restriction.
cc @FreddyLeaf
---
Full diff: https://github.com/llvm/llvm-project/pull/81175.diff
4 Files Affected:
- (modified) clang/docs/ReleaseNotes.rst (+2)
- (modified) clang/lib/Basic/Targets/X86.h (-6)
- (modified) clang/test/Analysis/bitint-no-crash.c (+2-3)
- (modified) clang/test/CodeGen/ext-int-cc.c (+8-8)
``diff
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 32440ee64e3ebe..a52e0c0112ba2c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -239,6 +239,8 @@ AMDGPU Support
X86 Support
^^^
+- Revert _BitInt() supported max width increase, which does not work properly.
+
Arm and AArch64 Support
^^^
diff --git a/clang/lib/Basic/Targets/X86.h b/clang/lib/Basic/Targets/X86.h
index d2232c7d5275ab..7010c1fbb5a4ef 100644
--- a/clang/lib/Basic/Targets/X86.h
+++ b/clang/lib/Basic/Targets/X86.h
@@ -507,9 +507,6 @@ class LLVM_LIBRARY_VISIBILITY X86_32TargetInfo : public
X86TargetInfo {
ArrayRef getTargetBuiltins() const override;
bool hasBitIntType() const override { return true; }
- size_t getMaxBitIntWidth() const override {
-return llvm::IntegerType::MAX_INT_BITS;
- }
};
class LLVM_LIBRARY_VISIBILITY NetBSDI386TargetInfo
@@ -820,9 +817,6 @@ class LLVM_LIBRARY_VISIBILITY X86_64TargetInfo : public
X86TargetInfo {
ArrayRef getTargetBuiltins() const override;
bool hasBitIntType() const override { return true; }
- size_t getMaxBitIntWidth() const override {
-return llvm::IntegerType::MAX_INT_BITS;
- }
};
// x86-64 Windows target
diff --git a/clang/test/Analysis/bitint-no-crash.c
b/clang/test/Analysis/bitint-no-crash.c
index 0a367fa930dc9b..aa9bd61e7e421a 100644
--- a/clang/test/Analysis/bitint-no-crash.c
+++ b/clang/test/Analysis/bitint-no-crash.c
@@ -1,10 +1,9 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core \
// RUN: -analyzer-checker=debug.ExprInspection \
- // RUN: -triple x86_64-pc-linux-gnu \
+ // RUN: -fexperimental-max-bitint-width=256 \
// RUN: -verify %s
-// Don't crash when using _BitInt(). Pin to the x86_64 triple for now,
-// since not all architectures support _BitInt()
+// Don't crash when using _BitInt().
// expected-no-diagnostics
_BitInt(256) a;
_BitInt(129) b;
diff --git a/clang/test/CodeGen/ext-int-cc.c b/clang/test/CodeGen/ext-int-cc.c
index 001e866d34b45e..b233285ea36da3 100644
--- a/clang/test/CodeGen/ext-int-cc.c
+++ b/clang/test/CodeGen/ext-int-cc.c
@@ -131,10 +131,10 @@ void ParamPassing3(_BitInt(15) a, _BitInt(31) b) {}
// are negated. This will give an error when a target does support larger
// _BitInt widths to alert us to enable the test.
void ParamPassing4(_BitInt(129) a) {}
-// LIN64: define{{.*}} void @ParamPassing4(ptr byval(i129) align 8 %{{.+}})
-// WIN64: define dso_local void @ParamPassing4(ptr %{{.+}})
-// LIN32: define{{.*}} void @ParamPassing4(ptr %{{.+}})
-// WIN32: define dso_local void @ParamPassing4(ptr %{{.+}})
+// LIN64-NOT: define{{.*}} void @ParamPassing4(ptr byval(i129) align 8 %{{.+}})
+// WIN64-NOT: define dso_local void @ParamPassing4(ptr %{{.+}})
+// LIN32-NOT: define{{.*}} void @ParamPassing4(ptr %{{.+}})
+// WIN32-NOT: define dso_local void @ParamPassing4(ptr %{{.+}})
// NACL-NOT: define{{.*}} void @ParamPassing4(ptr byval(i129) align 8 %{{.+}})
// NVPTX64-NOT: define{{.*}} void @ParamPassing4(ptr byval(i129) align 8
%{{.+}})
// NVPTX-NOT: define{{.*}} void @ParamPassing4(ptr byval(i129) align 8 %{{.+}})
@@ -290,10 +290,10 @@ _BitInt(128) ReturnPassing4(void){}
#if __BITINT_MAXWIDTH__ > 128
_BitInt(129) ReturnPassing5(void){}
-// LIN64: define{{.*}} void @ReturnPassing5(ptr dead_on_unwind noalias
writable sret
-// WIN64: define dso_local void @ReturnPassing5(ptr dead_on_unwind noalias
writable sret
-// LIN32: define{{.*}} void @ReturnPassing5(ptr dead_on_unwind noalias
writable sret
-// WIN32: define dso_local void @ReturnPassing5(ptr dead_on_unwind noalias
writable sret
+// LIN64-NOT: define{{.*}} void @ReturnPassing5(ptr dead_on_unwind noalias
writable sret
+//
[clang] Revert "[X86][clang] Lift _BitInt() supported max width." (PR #81175)
https://github.com/hvdijk created
https://github.com/llvm/llvm-project/pull/81175
This reverts commit def720726b73e0d7ab139376ab3ea955f25f4d89.
As noted in #60925 and in D86310, with the current implementation of `_BitInt`
in Clang, we can have either a correct `__int128` implementation, or a correct
`_BitInt(128)` implementation, but not both: having a correct `__int128`
implementation results in a mostly-working `_BitInt(65-128)` type, but internal
compiler errors and seriously wrong code for `_BitInt(129+)`. The currently
specified ABI is not implementable on top of LLVM's integer types, and attempts
to get the ABI changed to something we can implement have resulted in nothing,
so I think the safest thing to do at this time is to make sure Clang reports
this as an error in exactly the same way that it does for all other
architectures. I think it was simply too soon to lift that restriction.
cc @FreddyLeaf
>From 7b8f198b3bb00ca0624e7de061a977e1d3b87c00 Mon Sep 17 00:00:00 2001
From: Harald van Dijk
Date: Thu, 8 Feb 2024 19:06:14 +
Subject: [PATCH] Revert "[X86][clang] Lift _BitInt() supported max width."
This reverts commit def720726b73e0d7ab139376ab3ea955f25f4d89.
---
clang/docs/ReleaseNotes.rst | 2 ++
clang/lib/Basic/Targets/X86.h | 6 --
clang/test/Analysis/bitint-no-crash.c | 5 ++---
clang/test/CodeGen/ext-int-cc.c | 16
4 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 32440ee64e3eb..a52e0c0112ba2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -239,6 +239,8 @@ AMDGPU Support
X86 Support
^^^
+- Revert _BitInt() supported max width increase, which does not work properly.
+
Arm and AArch64 Support
^^^
diff --git a/clang/lib/Basic/Targets/X86.h b/clang/lib/Basic/Targets/X86.h
index d2232c7d5275a..7010c1fbb5a4e 100644
--- a/clang/lib/Basic/Targets/X86.h
+++ b/clang/lib/Basic/Targets/X86.h
@@ -507,9 +507,6 @@ class LLVM_LIBRARY_VISIBILITY X86_32TargetInfo : public
X86TargetInfo {
ArrayRef getTargetBuiltins() const override;
bool hasBitIntType() const override { return true; }
- size_t getMaxBitIntWidth() const override {
-return llvm::IntegerType::MAX_INT_BITS;
- }
};
class LLVM_LIBRARY_VISIBILITY NetBSDI386TargetInfo
@@ -820,9 +817,6 @@ class LLVM_LIBRARY_VISIBILITY X86_64TargetInfo : public
X86TargetInfo {
ArrayRef getTargetBuiltins() const override;
bool hasBitIntType() const override { return true; }
- size_t getMaxBitIntWidth() const override {
-return llvm::IntegerType::MAX_INT_BITS;
- }
};
// x86-64 Windows target
diff --git a/clang/test/Analysis/bitint-no-crash.c
b/clang/test/Analysis/bitint-no-crash.c
index 0a367fa930dc9..aa9bd61e7e421 100644
--- a/clang/test/Analysis/bitint-no-crash.c
+++ b/clang/test/Analysis/bitint-no-crash.c
@@ -1,10 +1,9 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core \
// RUN: -analyzer-checker=debug.ExprInspection \
- // RUN: -triple x86_64-pc-linux-gnu \
+ // RUN: -fexperimental-max-bitint-width=256 \
// RUN: -verify %s
-// Don't crash when using _BitInt(). Pin to the x86_64 triple for now,
-// since not all architectures support _BitInt()
+// Don't crash when using _BitInt().
// expected-no-diagnostics
_BitInt(256) a;
_BitInt(129) b;
diff --git a/clang/test/CodeGen/ext-int-cc.c b/clang/test/CodeGen/ext-int-cc.c
index 001e866d34b45..b233285ea36da 100644
--- a/clang/test/CodeGen/ext-int-cc.c
+++ b/clang/test/CodeGen/ext-int-cc.c
@@ -131,10 +131,10 @@ void ParamPassing3(_BitInt(15) a, _BitInt(31) b) {}
// are negated. This will give an error when a target does support larger
// _BitInt widths to alert us to enable the test.
void ParamPassing4(_BitInt(129) a) {}
-// LIN64: define{{.*}} void @ParamPassing4(ptr byval(i129) align 8 %{{.+}})
-// WIN64: define dso_local void @ParamPassing4(ptr %{{.+}})
-// LIN32: define{{.*}} void @ParamPassing4(ptr %{{.+}})
-// WIN32: define dso_local void @ParamPassing4(ptr %{{.+}})
+// LIN64-NOT: define{{.*}} void @ParamPassing4(ptr byval(i129) align 8 %{{.+}})
+// WIN64-NOT: define dso_local void @ParamPassing4(ptr %{{.+}})
+// LIN32-NOT: define{{.*}} void @ParamPassing4(ptr %{{.+}})
+// WIN32-NOT: define dso_local void @ParamPassing4(ptr %{{.+}})
// NACL-NOT: define{{.*}} void @ParamPassing4(ptr byval(i129) align 8 %{{.+}})
// NVPTX64-NOT: define{{.*}} void @ParamPassing4(ptr byval(i129) align 8
%{{.+}})
// NVPTX-NOT: define{{.*}} void @ParamPassing4(ptr byval(i129) align 8 %{{.+}})
@@ -290,10 +290,10 @@ _BitInt(128) ReturnPassing4(void){}
#if __BITINT_MAXWIDTH__ > 128
_BitInt(129) ReturnPassing5(void){}
-// LIN64: define{{.*}} void @ReturnPassing5(ptr dead_on_unwind noalias
writable sret
-// WIN64: define dso_local void @ReturnPassing5(ptr dead_on_unwind noalias
writable sret
-// LIN32: define{{.*}} void @ReturnPassing5(ptr dead_on_unwind noalias
