[NFC] Fix the definition of SuitableAlign

[Xiangling_L]

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.

[jyknight]

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]

In D88659#2306403, @jyknight wrote:

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.

[jyknight]

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?

[hubert.reinterpretcast]

In D88659#2318203, @jyknight wrote:

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.

[jyknight]

In D88659#2319636, @hubert.reinterpretcast wrote:

In D88659#2318203, @jyknight wrote:

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.

[hubert.reinterpretcast]

In D88659#2323631, @jyknight wrote:

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?

[Xiangling_L]

ping.

[Xiangling_L]

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

[jyknight]

In D88659#2339357, @Xiangling_L wrote:

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.

[Xiangling_L]

Edit the definition of SuitableAlign;

[hubert.reinterpretcast]

LGTM; thanks.

[Xiangling_L]

Manually close the revision after landing it upstream.

clang/include/clang/Basic/TargetInfo.h

public:
   virtual bool hasFloat16Type() const { return HasFloat16; }
 
   /// Determine whether the _BFloat16 type is supported on this target.
   virtual bool hasBFloat16Type() const { return HasBFloat16; }
 
   /// Determine whether constrained floating point is supported on this target.
   virtual bool hasStrictFP() const { return HasStrictFP; }
 
-  /// Return the alignment that is suitable for storing any
-  /// object with a fundamental alignment requirement.
+  /// Return the alignment that is the largest alignment ever used for any
+  /// scalar/SIMD data type on the target machine you are compiling for
+  /// (including types with an extended alignment requirement).
   unsigned getSuitableAlign() const { return SuitableAlign; }
 
   /// Return the default alignment for __attribute__((aligned)) on
   /// this target, to be used if no alignment value is specified.
   unsigned getDefaultAlignForAttributeAligned() const {
     return DefaultAlignForAttributeAligned;
   }