Page MenuHomePhabricator

[NFC] Fix the definition of SuitableAlign
ClosedPublic

Authored by Xiangling_L on Thu, Oct 1, 7:51 AM.

Details

Summary

There are only two places where "SuitableAlign" is used:

  • calculate 'BIGGEST_ALIGNMENT' macro
  • alignment for 'Alloca' Inst

The precedent set by GCC appears to be that BIGGEST_ALIGNMENT and alloca both should follow vector alignment requirements of the selected ISA even when that is greater than any fundamental alignment requirement, and greater than malloc's alignment.

Diff Detail

Event Timeline

Xiangling_L created this revision.Thu, Oct 1, 7:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Oct 1, 7:51 AM
Xiangling_L requested review of this revision.Thu, Oct 1, 7:51 AM

Hm, to start with, the current state of this confuses me.

In GCC, the preprocessor macro __BIGGEST_ALIGNMENT__ was supposed to expose the alignment used by __attribute__((aligned)) (no arg specified), as well the alignment used for alloca. However, this is no longer the case on x86: BIGGEST_ALIGNMENT is 512bits with avx-512 enabled, 256bits with avx enabled, and otherwise 128bits. Alloca follows this too. But, __attribute__((aligned)) was fixed at 128bit alignment, regardless of AVX being enabled, in order to not break ABI compatibility with structs using that. On other architectures, the 3 values seem to be always the same.

In clang, we similarly have (before this patch) both DefaultAlignForAttributeAligned (used for `attribute((aligned))), and SuitableAlign (used for the predefined __BIGGEST_ALIGNMENT__ and alignment for alloca). But these values are different on very many architectures...which I think is probably wrong. Furthermore, SuitableAlign isn't being adjusted to be suitable for vectors, like it is in gcc, which _also_ seems wrong. Looks like there's actually an earlier patch to fix that which was never merged: https://reviews.llvm.org/D39313

So, anyways -- back to this patch: On AIX PPC, you want alloca to align to 128bits, __attribute__((aligned)) to align to 128bits (aka 8 bytes), but __BIGGEST_ALIGNMENT__ to only be 4?

That seems pretty weird, and probably wrong?

Xiangling_L added a comment.EditedMon, Oct 5, 10:47 AM

Hm, to start with, the current state of this confuses me.

In GCC, the preprocessor macro __BIGGEST_ALIGNMENT__ was supposed to expose the alignment used by __attribute__((aligned)) (no arg specified), as well the alignment used for alloca. However, this is no longer the case on x86: BIGGEST_ALIGNMENT is 512bits with avx-512 enabled, 256bits with avx enabled, and otherwise 128bits. Alloca follows this too. But, __attribute__((aligned)) was fixed at 128bit alignment, regardless of AVX being enabled, in order to not break ABI compatibility with structs using that. On other architectures, the 3 values seem to be always the same.

In clang, we similarly have (before this patch) both DefaultAlignForAttributeAligned (used for `attribute((aligned))), and SuitableAlign (used for the predefined __BIGGEST_ALIGNMENT__ and alignment for alloca). But these values are different on very many architectures...which I think is probably wrong. Furthermore, SuitableAlign isn't being adjusted to be suitable for vectors, like it is in gcc, which _also_ seems wrong. Looks like there's actually an earlier patch to fix that which was never merged: https://reviews.llvm.org/D39313

So, anyways -- back to this patch: On AIX PPC, you want alloca to align to 128bits, __attribute__((aligned)) to align to 128bits (aka 8 bytes), but __BIGGEST_ALIGNMENT__ to only be 4?

That seems pretty weird, and probably wrong?

As you mentioned, without this patch, SuitableAlign is used for the predefined __BIGGEST_ALIGNMENT__ and alignment for alloca. But on AIX, the BIGGEST_ALIGNMENT should be 8bytes, alloca and __attribute__((aligned)) should align to 16bytes considering vector types which have to be aligned to 16bytes, that's why we want to split SuitableAlign following current Clang state. We'd like to split something out to consider vector type alignment.

Thanks for pointing out the current state on GCC and Clang, I also hope someone else can shine a light on this.

As you mentioned, without this patch, SuitableAlign is used for the predefined __BIGGEST_ALIGNMENT__ and alignment for alloca. But on AIX, the BIGGEST_ALIGNMENT should be 8bytes, alloca and __attribute__((aligned)) should align to 16bytes considering vector types which have to be aligned to 16bytes, that's why we want to split SuitableAlign following current Clang state. We'd like to split something out to consider vector type alignment.

It seems like on AIX, __BIGGEST_ALIGNMENT__ should just be set to 16, then. I'm not sure why you want it to be 8?

It seems like on AIX, __BIGGEST_ALIGNMENT__ should just be set to 16, then. I'm not sure why you want it to be 8?

/// Return the alignment that is suitable for storing any
/// object with a fundamental alignment requirement.

Vector types have extended (not fundamental) alignment on AIX, because malloc is not guaranteed to return addresses that are 16-byte aligned.

It seems like on AIX, __BIGGEST_ALIGNMENT__ should just be set to 16, then. I'm not sure why you want it to be 8?

/// Return the alignment that is suitable for storing any
/// object with a fundamental alignment requirement.

Vector types have extended (not fundamental) alignment on AIX, because malloc is not guaranteed to return addresses that are 16-byte aligned.

ISTM that comment may be incorrect. At least, the precedent set by GCC appears to be that BIGGEST_ALIGNMENT and alloca _both_ should follow vector alignment requirements of the selected ISA even when that is greater than any fundamental alignment requirement, and greater than malloc's alignment.

ISTM that comment may be incorrect. At least, the precedent set by GCC appears to be that BIGGEST_ALIGNMENT and alloca _both_ should follow vector alignment requirements of the selected ISA even when that is greater than any fundamental alignment requirement, and greater than malloc's alignment.

That would make sense. I guess this is really the alloca alignment value then? We can change the scope of this patch to simply correct the comment and add another patch to update the value on AIX to be 16. Do you believe the change to the "definition" of SuitableAlign needs a note to the mailing list?

Hi @jyknight , are you okay with us changing the "definition" of SuitableAlign without sending a note to the mailing list?

Hi @jyknight , are you okay with us changing the "definition" of SuitableAlign without sending a note to the mailing list?

Yes, I think that it is fine to adjust the comment just in this code-review.

Edit the definition of SuitableAlign;

Xiangling_L retitled this revision from [FE]Split SuitableAlign into two parts to [NFC] Fix the definition of SuitableAlign.Tue, Oct 20, 10:24 AM
Xiangling_L edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Wed, Oct 21, 7:16 AM
Xiangling_L closed this revision.Thu, Oct 22, 1:13 PM

Manually close the revision after landing it upstream.