[PATCH] D88659: [FE]Split SuitableAlign into two parts

Mon Oct 5 10:48:01 PDT 2020

Xiangling_L added a comment.

In D88659#2306403 <https://reviews.llvm.org/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`.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88659/new/

https://reviews.llvm.org/D88659