Align i128 to 16 bytes

[nagisa]

Summary

This is what clang does as can be seen in [1]. As Clang does not support using 128-bit integers on
non-64 bit targets, no effort was spent on updating the data-layouts for 32-bit only backends.

This is wanted for rustc, which wants data-layout used by LLVM and data-layout used by rustc
match to some extent.

[1]: https://github.com/llvm-mirror/clang/blob/916645c2637f8c80948eedcdea02258d0a6f79f5/lib/AST/ASTContext.cpp#L1734-L1738

[chfast]

Does this affects also bigger types like i256?

[nagisa]

Does this affects also bigger types like i256?

Yes, it does, as LLVM will pick the alignment information of the largest specified integer. So now that largest specified integer type is i128, and not i64 as it was previously, i256, i512, i1024, etc types will be using alignment of i128. This is also evidenced by the test changes to mul-i{512,1024} tests for x86 backend.

[joerg]

Why? At least for some x86 targets, this will unncessarily penalize the stack. Don't know all the other 32bit architectures, but many of those don't have a stack alignment larger than 32bit either.

[nagisa]

At least for some x86 targets, this will unncessarily penalize the stack. Don't know all the other 32bit architectures, but many of those don't have a stack alignment larger than 32bit either.

I’m willing to adjust the PR to only touch 64-bit targets. For the 64-bit targets 128-bit alignment is necessary for correct FFI with Clang-generated code, which generates code with assumption that i128 is 128-aligned always.

[nagisa]

Make alignment changes specific to 64-bit targets.

[chfast]

Will not that penalize stack usage on 64-bit targets as well?

[joerg]

What is the motivation behind requiring 128bit alignment? E.g. normal lowering of __uint128_t arithmetic on x86_64 will use uint64_t arithmetic and naturally require the same alignment.

I'm concerned about 32bit platforms because I really would like to see the artifical restriction of __uint128_t to 64bit targets be removed at some point, so that code can naturally depend on them everywhere.

[majnemer]

In D28990#652740, @joerg wrote:

What is the motivation behind requiring 128bit alignment? E.g. normal lowering of __uint128_t arithmetic on x86_64 will use uint64_t arithmetic and naturally require the same alignment.

I'm concerned about 32bit platforms because I really would like to see the artifical restriction of __uint128_t to 64bit targets be removed at some point, so that code can naturally depend on them everywhere.

The x86_64 psABI specifies an alignment of 16 bytes for int128_t and uint128_t.

[nagisa]

Its as @majnemer says, 16-byte alignment is required by ABI. Not just targets which use SysV, but any, where int128 is supported, because clang hardcodes int128 to have alignment of 16-bytes on *all* targets.

I’m surprised y'all are debating efficiency when its a plain correctness issue. IMHO what should be done is having the correctness issue fixed (this patch), then maybe some work towards removing the special case from clang (in the code I linked above) and finally incremental efficiency improvements where they are applicable.

[joerg]

You make it sound as if the current alignment decision was made intentional. I strongly doubt that. So yes, please double check all the ABIs here to make sure that the wild card code is actually correct.

[nagisa]

please double check all the ABIs here

Sorry, I do not have the time on my hands to go and find and parse ABI documents for literally every target triplet LLVM supports, so here’s what I know instead:

• x86_64 SysV - 16 byte aligned;
• x64 (msvc) ABI - 16 byte aligned;
• ARM - 16 byte aligned;
• PPC64 - quadword, so 16 byte;

Sounds intentional enough to me at least.

[majnemer]

Yes, I think this LGTM if you can find rationale in the other psABI documents. https://www.uclibc.org/docs/psABI-ppc64.pdf gives the PPC64 int128 alignment as 16 bytes. Can you find justifications for i128 for the other platforms?

[nagisa]

This is for ARM: http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055c/IHI0055C_beta_aapcs64.pdf;
This is for x64: https://msdn.microsoft.com/en-us/library/zthk2dkh.aspx (protip: needs careful reading because __int128 is not supported by MSVC directly and is not explicitly mentioned by the docs, but wording is unambiguous enough to make conclusions);
SPARC (https://docs.oracle.com/cd/E26502_01/pdf/E28387.pdf) does not mention anything really and there’s some encryption involving “slots” and whatnot in that document, but

For 64-bit code, double and long long datat types occupy just one slot, but long double and double complex data occupy two slots, and these slots are aligned (slot number modulo 2 equals 0), skipping a slot if necessary for alignment.

seems to indicate that stuff ought to be aligned to 2 times of whatever slots ought to mean;

[echristo]

This LGTM as well. At the very least I believe it is intentional. This is, at a minimum, correct for x86-64 and ppc64.

[nagisa]

When can I expect this to land into the trunk?

[echristo]

When you mention that you need someone to land it for you. I'll do it shortly.

[mehdi_amini]

In D28990#659198, @nagisa wrote:

When can I expect this to land into the trunk?

You need to ask for it, reviewers usually assume you have commit right unless mentioned otherwise. So when someone approves your patch just ask "can you land this for me please".

[echristo]

This patch didn't pass either the llvm testsuite or the clang testsuite. I've worked up patches for both, but there are a couple of places I think we're changing the ABI (via changes in clang that I had to make) that I'm a bit more uncomfortable with - I'll look at this over the next couple of days.

[echristo]

OK. I've committed x86-64 linux and ppc64 linux support of this patch. We should get additional confirmation on the other platforms before we change things. I believe it's correct in all cases, but I'm going to be a bit conservative here.

Thanks!

-eric

[vchuravy]

@echristo I noticed that you reverted r294702 shortly after it landed. What needs to be done to get it working?

[echristo]

In D28990#801641, @vchuravy wrote:

@echristo I noticed that you reverted r294702 shortly after it landed. What needs to be done to get it working?

From the revert:

commit 8c54b816e554ff9f9a75ec8a1d50198d2bfd9633
Author: Eric Christopher <[e-mail]>
Date: Fri Feb 10 04:35:32 2017 +0000

Temporarily revert "For X86-64 linux and PPC64 linux align int128 to 16 bytes."
until we can get better TargetMachine::isCompatibleDataLayout to compare - otherwise
we can't code generate existing bitcode without a string equality data layout.

This reverts commit r294702.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@294709 91177308-0d34-0410-b5e6-96231b3b80d8

lib/Target/Mips/MipsTargetMachine.cpp

static std::string computeDataLayout(const Triple &TT, StringRef CPU,
   // Pointers are 32 bit on some ABIs.
   if (!ABI.IsN64())
     Ret += "-p:32:32";
 
   // 8 and 16 bit integers only need to have natural alignment, but try to
   // align them to 32 bits. 64 bit integers have natural alignment.
   Ret += "-i8:8:32-i16:16:32-i64:64";
 
+  // 128 bit integers are always aligned to 128 bits, but only 64-bit matters,
+  // because __int128 is only supoprted on 64-bit targets.
+  if (ABI.IsN64() || ABI.IsN32())
+    Ret += "-i128:128";
+
   // 32 bit registers are always available and the stack is at least 64 bit
   // aligned. On N64 64 bit registers are also available and the stack is
   // 128 bit aligned.
   if (ABI.IsN64() || ABI.IsN32())
     Ret += "-n32:64-S128";
   else
     Ret += "-n32-S64";
 

lib/Target/PowerPC/PPCTargetMachine.cpp

static std::string getDataLayoutString(const Triple &T) {
 
   // Note, the alignment values for f64 and i64 on ppc64 in Darwin
   // documentation are wrong; these are correct (i.e. "what gcc does").
   if (is64Bit || !T.isOSDarwin())
     Ret += "-i64:64";
   else
     Ret += "-f64:32:64";
 
+  // 128 bit integers are always aligned to 128 bits, but only 64-bit matters,
+  // because __int128 is only supoprted on 64-bit targets.
+  if (is64Bit)
+    Ret += "-i128:128";
+
   // PPC64 has 32 and 64 bit registers, PPC32 has only 32 bit ones.
   if (is64Bit)
     Ret += "-n32:64";
   else
     Ret += "-n32";
 
   return Ret;
 }

lib/Target/Sparc/SparcTargetMachine.cpp

static std::string computeDataLayout(const Triple &T, bool is64Bit) {
 
   // Some ABIs have 32bit pointers.
   if (!is64Bit)
     Ret += "-p:32:32";
 
   // Alignments for 64 bit integers.
   Ret += "-i64:64";
 
+  // 128 bit integers are always aligned to 128 bits, but only 64-bit matters,
+  // because __int128 is only supoprted on 64-bit targets.
+  if (is64Bit)
+    Ret += "-i128:128";
+
   // On SparcV9 128 floats are aligned to 128 bits, on others only to 64.
   // On SparcV9 registers can hold 64 or 32 bits, on others only 32.
   if (is64Bit)
     Ret += "-n32:64";
   else
     Ret += "-f128:64-n32";
 
   if (is64Bit)

lib/Target/SystemZ/SystemZTargetMachine.cpp

static std::string computeDataLayout(const Triple &TT, StringRef CPU,
   // Data mangling.
   Ret += DataLayout::getManglingComponent(TT);
 
   // Make sure that global data has at least 16 bits of alignment by
   // default, so that we can refer to it using LARL.  We don't have any
   // special requirements for stack variables though.
   Ret += "-i1:8:16-i8:8:16";
 
-  // 64-bit integers are naturally aligned.
-  Ret += "-i64:64";
+  // 64-bit and 128-bit integers are naturally aligned.
+  Ret += "-i64:64-i128:128";
 
   // 128-bit floats are aligned only to 64 bits.
   Ret += "-f128:64";
 
   // When using the vector ABI, 128-bit vectors are also aligned to 64 bits.
   if (VectorABI)
     Ret += "-v128:64";
 

lib/Target/X86/X86TargetMachine.cpp

static std::string computeDataLayout(const Triple &TT) {
   // Some ABIs align 64 bit integers and doubles to 64 bits, others to 32.
   if (TT.isArch64Bit() || TT.isOSWindows() || TT.isOSNaCl())
     Ret += "-i64:64";
   else if (TT.isOSIAMCU())
     Ret += "-i64:32-f64:32";
   else
     Ret += "-f64:32:64";
 
+  // 128 bit integers are always aligned to 128 bits, but only 64-bit matters,
+  // because __int128 is only supoprted on 64-bit targets.
+  if (TT.isArch64Bit())
+    Ret += "-i128:128";
+
   // Some ABIs align long double to 128 bits, others to 32.
   if (TT.isOSNaCl() || TT.isOSIAMCU())
     ; // No f80
   else if (TT.isArch64Bit() || TT.isOSDarwin())
     Ret += "-f80:128";
   else
     Ret += "-f80:32";
 

test/CodeGen/X86/catchpad-dynamic-alloca.ll

 
 catch.switch:
   %cs = catchswitch within none [label %catch.pad] unwind to caller
 }
 
 ; CHECK-LABEL: $handlerMap$0$test2:
 ; CHECK:      .long   0
 ; CHECK-NEXT: .long   0
-; CHECK-NEXT: .long   8
+; CHECK-NEXT: .long   16

test/CodeGen/X86/osx-private-labels.ll

 
 @private5 = private unnamed_addr constant i64 42
 ; CHECK: .section __TEXT,__literal8,8byte_literals
 ; CHECK-NEXT: .p2align  3
 ; CHECK-NEXT: L_private5:
 
 @private6 = private unnamed_addr constant i128 42
 ; CHECK: .section __TEXT,__literal16,16byte_literals
-; CHECK-NEXT: .p2align  3
+; CHECK-NEXT: .p2align  4
 ; CHECK-NEXT: L_private6:
 
 %struct._objc_class = type { i8* }
 @private7 = private global %struct._objc_class* null, section "__OBJC,__cls_refs,literal_pointers,no_dead_strip"
 ; CHECK: .section __OBJC,__cls_refs,literal_pointers,no_dead_strip
 ; CHECK: .p2align 3
 ; CHECK: L_private7: