[Clang][AArch64] Fixed incorrect _BitInt alignment #90602

Lukacma · 2024-04-30T12:57:37Z

This patch makes determining alignment and width of BitInt to be target ABI specific and makes it consistent with Procedure Call Standard for the Arm® 64-bit Architecture (AArch64) for AArch64 targets.

llvmbot · 2024-04-30T12:58:16Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-aarch64

Author: None (Lukacma)

Changes

This patch makes determining alignment and width of BitInt to be target ABI specific and makes it consistent with Procedure Call Standard for the Arm® 64-bit Architecture (AArch64) for AArch64 targets.

Full diff: https://github.com/llvm/llvm-project/pull/90602.diff

5 Files Affected:

(modified) clang/include/clang/Basic/TargetInfo.h (+8)
(modified) clang/lib/AST/ASTContext.cpp (+2-3)
(modified) clang/lib/Basic/Targets/AArch64.cpp (+5-1)
(modified) clang/lib/Basic/Targets/AArch64.h (+3)
(modified) clang/test/CodeGen/aapcs64-align.cpp (+63-1)

diff --git a/clang/include/clang/Basic/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index 3ced2e7397a754..1b5efa488b6ded 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -518,6 +518,14 @@ class TargetInfo : public TransferrableTargetInfo,
   /// getInt128Align() - Returns the alignment of Int128.
   unsigned getInt128Align() const { return Int128Align; }
 
+  /// getBitIntAlign/Width - Return aligned size of '_BitInt' and 
+  /// 'unsigned _BitInt' for this target, in bits.
+  unsigned getBitIntWidth(unsigned NumBits) const {
+    return llvm::alignTo(NumBits, getBitIntAlign(NumBits));}
+  virtual unsigned getBitIntAlign(unsigned NumBits) const {
+    return std::clamp<unsigned>(llvm::PowerOf2Ceil(NumBits), getCharWidth(), 
+                                getLongLongAlign());}
+
   /// getShortAccumWidth/Align - Return the size of 'signed short _Accum' and
   /// 'unsigned short _Accum' for this target, in bits.
   unsigned getShortAccumWidth() const { return ShortAccumWidth; }
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index cbf4932aff9a6b..f440af50e08a49 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -2263,9 +2263,8 @@ TypeInfo ASTContext::getTypeInfoImpl(const Type *T) const {
   }
   case Type::BitInt: {
     const auto *EIT = cast<BitIntType>(T);
-    Align = std::clamp<unsigned>(llvm::PowerOf2Ceil(EIT->getNumBits()),
-                                 getCharWidth(), Target->getLongLongAlign());
-    Width = llvm::alignTo(EIT->getNumBits(), Align);
+    Align = Target->getBitIntAlign(EIT->getNumBits());
+    Width = Target->getBitIntWidth(EIT->getNumBits());
     break;
   }
   case Type::Record:
diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index c8d243a8fb7aea..9a6c197ff77b58 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -1473,6 +1473,10 @@ bool AArch64TargetInfo::validatePointerAuthKey(
 
 bool AArch64TargetInfo::hasInt128Type() const { return true; }
 
+unsigned AArch64TargetInfo::getBitIntAlign(unsigned NumBits) const{
+        return std::clamp<unsigned>(llvm::PowerOf2Ceil(NumBits), getCharWidth(), 
+                                    getInt128Align());}
+
 AArch64leTargetInfo::AArch64leTargetInfo(const llvm::Triple &Triple,
                                          const TargetOptions &Opts)
     : AArch64TargetInfo(Triple, Opts) {}
@@ -1674,4 +1678,4 @@ void RenderScript64TargetInfo::getTargetDefines(const LangOptions &Opts,
                                                 MacroBuilder &Builder) const {
   Builder.defineMacro("__RENDERSCRIPT__");
   AArch64leTargetInfo::getTargetDefines(Opts, Builder);
-}
+}
\ No newline at end of file
diff --git a/clang/lib/Basic/Targets/AArch64.h b/clang/lib/Basic/Targets/AArch64.h
index 12fb50286f7511..d8cdc814b2a9ae 100644
--- a/clang/lib/Basic/Targets/AArch64.h
+++ b/clang/lib/Basic/Targets/AArch64.h
@@ -202,6 +202,9 @@ class LLVM_LIBRARY_VISIBILITY AArch64TargetInfo : public TargetInfo {
   bool hasBitIntType() const override { return true; }
 
   bool validateTarget(DiagnosticsEngine &Diags) const override;
+
+  unsigned getBitIntAlign(unsigned NumBits) const override;
+ 
 };
 
 class LLVM_LIBRARY_VISIBILITY AArch64leTargetInfo : public AArch64TargetInfo {
diff --git a/clang/test/CodeGen/aapcs64-align.cpp b/clang/test/CodeGen/aapcs64-align.cpp
index de231f2123b975..7a8151022852ea 100644
--- a/clang/test/CodeGen/aapcs64-align.cpp
+++ b/clang/test/CodeGen/aapcs64-align.cpp
@@ -1,7 +1,7 @@
 // REQUIRES: arm-registered-target
 // RUN: %clang_cc1 -triple aarch64-none-elf \
 // RUN:   -O2 \
-// RUN:   -emit-llvm -o - %s | FileCheck %s
+// RUN:   -emit-llvm -fexperimental-max-bitint-width=1024 -o - %s | FileCheck %s
 
 extern "C" {
 
@@ -100,4 +100,66 @@ void f5m(int, int, int, int, int, P16);
 // CHECK: declare void @f5(i32 noundef, [2 x i64])
 // CHECK: declare void @f5m(i32 noundef, i32 noundef, i32 noundef, i32 noundef, i32 noundef, [2 x i64])
 
+//BitInt alignment
+struct BITINT129 {
+    char ch;
+    unsigned _BitInt(129) v;
+};
+
+int test_bitint129(){
+  return __builtin_offsetof(struct BITINT129, v);
 }
+// CHECK:  ret i32 16 
+
+struct BITINT127 {
+    char ch;
+    _BitInt(127) v;
+};
+
+int test_bitint127(){
+  return __builtin_offsetof(struct BITINT127, v);
+}
+// CHECK:  ret i32 16 
+
+struct BITINT63 {
+    char ch;
+    _BitInt(63) v;
+};
+
+int test_bitint63(){
+  return __builtin_offsetof(struct BITINT63, v);
+}
+// CHECK:  ret i32 8 
+
+struct BITINT32 {
+    char ch;
+    unsigned _BitInt(32) v;
+};
+
+int test_bitint32(){
+  return __builtin_offsetof(struct BITINT32, v);
+}
+// CHECK:  ret i32 4
+
+struct BITINT9 {
+    char ch;
+    unsigned _BitInt(9) v;
+};
+
+int test_bitint9(){
+  return __builtin_offsetof(struct BITINT9, v);
+}
+// CHECK:  ret i32 2
+
+struct BITINT8 {
+    char ch;
+    unsigned _BitInt(8) v;
+};
+
+int test_bitint8(){
+  return __builtin_offsetof(struct BITINT8, v);
+}
+// CHECK:  ret i32 1
+
+}
+

github-actions · 2024-04-30T13:00:22Z

✅ With the latest revision this PR passed the C/C++ code formatter.

efriedma-quic

LG

efriedma-quic · 2024-05-05T20:13:00Z

clang/lib/Basic/Targets/AArch64.cpp

@@ -1674,4 +1679,4 @@ void RenderScript64TargetInfo::getTargetDefines(const LangOptions &Opts,
                                                MacroBuilder &Builder) const {
  Builder.defineMacro("__RENDERSCRIPT__");
  AArch64leTargetInfo::getTargetDefines(Opts, Builder);
-}
+}


Missing newline?

efriedma-quic · 2024-05-05T20:21:28Z

clang/include/clang/Basic/TargetInfo.h

+  unsigned getBitIntWidth(unsigned NumBits) const {
+    return llvm::alignTo(NumBits, getBitIntAlign(NumBits));
+  }
+  virtual unsigned getBitIntAlign(unsigned NumBits) const {


Instead of making this virtual, maybe it makes sense to add a field std::optional<unsigned> BitIntMaxAlign to TargetInfo? I expect the logic here besides the max width to be consistent across all targets.

I am not sure I understand how this idea would work. Could you please elaborate on what do you mean ?

unsigned getBitIntMaxAlign() { return BitIntMaxAlign.or(LongLongAlign); } unsigned getBitIntAlign(unsigned NumBits) const { return std::clamp<unsigned>(llvm::PowerOf2Ceil(NumBits), getCharWidth(), getBitIntMaxAlign()); }

efriedma-quic

LGTM

[Clang][AArch64] Fixed incorrect _BitInt alignment

dfb8a9d

llvmbot added clang Clang issues not falling into any other category backend:AArch64 clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 30, 2024

Fix formatting

908693b

Lukacma requested review from momchil-velikov, hassnaaHamdi and CarolineConcatto April 30, 2024 13:17

Lukacma mentioned this pull request May 1, 2024

[Clang] Fix incorrect passing of _BitInt args #90741

Merged

Lukacma requested a review from AaronBallman May 3, 2024 12:52

efriedma-quic reviewed May 5, 2024

View reviewed changes

Lukacma added 2 commits May 8, 2024 14:55

add optional BitIntMaxAlign parameter to TargetInfo

e82b8a7

Update AArch64.cpp

22db03b

efriedma-quic approved these changes May 8, 2024

View reviewed changes

Merge branch 'main' into bitint-align

674a3d9

Lukacma merged commit 105dd60 into llvm:main May 9, 2024
3 of 4 checks passed

Lukacma deleted the bitint-align branch May 9, 2024 09:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang][AArch64] Fixed incorrect _BitInt alignment #90602

[Clang][AArch64] Fixed incorrect _BitInt alignment #90602

Lukacma commented Apr 30, 2024

llvmbot commented Apr 30, 2024 •

edited

Loading

github-actions bot commented Apr 30, 2024 •

edited

Loading

efriedma-quic left a comment

efriedma-quic May 5, 2024

Lukacma May 8, 2024

efriedma-quic May 5, 2024

Lukacma May 7, 2024

efriedma-quic May 7, 2024

Lukacma May 8, 2024

efriedma-quic left a comment

[Clang][AArch64] Fixed incorrect _BitInt alignment #90602

[Clang][AArch64] Fixed incorrect _BitInt alignment #90602

Conversation

Lukacma commented Apr 30, 2024

llvmbot commented Apr 30, 2024 • edited Loading

github-actions bot commented Apr 30, 2024 • edited Loading

efriedma-quic left a comment

Choose a reason for hiding this comment

efriedma-quic May 5, 2024

Choose a reason for hiding this comment

Lukacma May 8, 2024

Choose a reason for hiding this comment

efriedma-quic May 5, 2024

Choose a reason for hiding this comment

Lukacma May 7, 2024

Choose a reason for hiding this comment

efriedma-quic May 7, 2024

Choose a reason for hiding this comment

Lukacma May 8, 2024

Choose a reason for hiding this comment

efriedma-quic left a comment

Choose a reason for hiding this comment

llvmbot commented Apr 30, 2024 •

edited

Loading

github-actions bot commented Apr 30, 2024 •

edited

Loading