Page MenuHomePhabricator

Align i128 to 16 bytes
ClosedPublic

Authored by nagisa on Jan 22 2017, 6:19 AM.

Details

Reviewers
echristo
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

Diff Detail

Event Timeline

nagisa created this revision.Jan 22 2017, 6:19 AM
chfast added a subscriber: chfast.Jan 22 2017, 6:54 AM

Does this affects also bigger types like i256?

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 added a subscriber: joerg.Jan 22 2017, 9:46 AM

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.

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 updated this revision to Diff 85282.Jan 22 2017, 10:38 AM

Make alignment changes specific to 64-bit targets.

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

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.

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.

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 added a comment.Jan 24 2017, 3:54 PM

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.

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.

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?

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 accepted this revision.Jan 25 2017, 2:25 PM

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

This revision is now accepted and ready to land.Jan 25 2017, 2:25 PM

When can I expect this to land into the trunk?

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

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".

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.

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

echristo closed this revision.Feb 9 2017, 7:44 PM

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

@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 <echristo@gmail.com>
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