Revert "[X86][clang] Lift _BitInt() supported max width."

[hvdijk]

This reverts commit def7207.

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

[llvmbot]

@llvm/pr-subscribers-clang

Author: Harald van Dijk (hvdijk)

Changes

This reverts commit def7207.

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 --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<Builtin::Info> 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<Builtin::Info> 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
+// WIN64-NOT: define dso_local void @ReturnPassing5(ptr dead_on_unwind noalias writable sret
+// LIN32-NOT: define{{.*}} void @ReturnPassing5(ptr dead_on_unwind noalias writable sret
+// WIN32-NOT: define dso_local void @ReturnPassing5(ptr dead_on_unwind noalias writable sret
 // NACL-NOT: define{{.*}} void @ReturnPassing5(ptr dead_on_unwind noalias writable sret
 // NVPTX64-NOT: define{{.*}} i129 @ReturnPassing5(
 // NVPTX-NOT: define{{.*}} i129 @ReturnPassing5(

[AaronBallman]

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?

[AaronBallman]

CC @phoebewang as well

[nikic]

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?

[hvdijk]

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<NoUniqueAddressAttr>() && "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 <source>
1.	<eof> parser at end of file
2.	<source>:9:3: LLVM IR generation of declaration 'g'
3.	<source>:9:3: Generating code for declaration 'g'
 #0 0x000000000389ed98 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x389ed98)
 #1 0x000000000389ca7c llvm::sys::CleanupOnSignal(unsigned long) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x389ca7c)
 #2 0x00000000037e4f58 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0
 #3 0x00007f7cec242520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
 #4 0x00007f7cec2969fc pthread_kill (/lib/x86_64-linux-gnu/libc.so.6+0x969fc)
 #5 0x00007f7cec242476 gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x42476)
 #6 0x00007f7cec2287f3 abort (/lib/x86_64-linux-gnu/libc.so.6+0x287f3)
 #7 0x00007f7cec22871b (/lib/x86_64-linux-gnu/libc.so.6+0x2871b)
 #8 0x00007f7cec239e96 (/lib/x86_64-linux-gnu/libc.so.6+0x39e96)
 #9 0x0000000003baa784 (anonymous namespace)::CGRecordLowering::clipTailPadding() CGRecordLayoutBuilder.cpp:0:0
#10 0x0000000003bb0253 (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.

Context was provided

[nikic]

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.

[hvdijk]

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. (Edit: A direct load or store appears to not touch any padding, but it would be different if it's part of a struct -- except that in that case, we get that compiler crash before it gets to the wrong code for that.)

[nikic]

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.

[hvdijk]

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.

[rjmccall]

There's no such thing as "this is impossible to do". Clang, as the frontend, is responsible for emitting IR that gets the effect we want. If that means contorting the IR we generate to do ugly things like always doing loads and stores in a wider type or type-punning through a temporary in order to pass as a different type, that's just life in the frontend. This alloc-size problem sounds like we just need to not use these types as struct fields when lowering to IR types, which is annoying but, again, 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.

As far as reversion goes, it sounds like we have serious bugs, and we can't really promise ABI interoperability with previous compilers. So it goes. Let's figure out what behavior we want and then figure out how to get that behavior from LLVM, one way or the other.

[hvdijk]

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.

[hvdijk]

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.