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

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 299404.
Xiangling_L added a comment.
Herald added a subscriber: dexonsmith.

Edit the definition of `SuitableAlign`;


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

https://reviews.llvm.org/D88659

Files:
  clang/include/clang/Basic/TargetInfo.h


Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -581,8 +581,9 @@
   /// 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


Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -581,8 +581,9 @@
   /// 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88659

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88659

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added a comment.

ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88659

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88659

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88659

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88659

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88659

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88659

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88659

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L created this revision.
Xiangling_L added reviewers: hubert.reinterpretcast, zarko, Jason, jyknight, 
efriedma.
Herald added subscribers: cfe-commits, luismarques, apazos, sameer.abuasal, 
pzheng, s.egerton, lenary, Jim, jocewei, PkmX, the_o, brucehoult, 
MartinMosbeck, rogfer01, atanasyan, edward-jones, zzheng, jrtc27, niosHD, 
sabuasal, simoncook, johnrusso, rbar, asb, fedor.sergeev, kbarton, 
jgravelle-google, sbc100, nemanjai, sdardis, dylanmckay, dschuff.
Herald added a project: clang.
Xiangling_L requested review of this revision.
Herald added subscribers: MaskRay, aheejin.

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

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

On some targets, like AIX, the two value are not equal. So we split 
`SuitableAlign` into two parts to meet the needs.

One is 'GuanranteedAlign'[?any better name] which is used to calculate 
'BIGGEST_ALIGNMENT' and represent fundamental alignment requirement; the other 
one is still 'SuitableAlign' used to calculate alignment for 'Alloca' Inst.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88659

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/AArch64.cpp
  clang/lib/Basic/Targets/ARC.h
  clang/lib/Basic/Targets/ARM.cpp
  clang/lib/Basic/Targets/AVR.h
  clang/lib/Basic/Targets/MSP430.h
  clang/lib/Basic/Targets/Mips.h
  clang/lib/Basic/Targets/PPC.h
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Basic/Targets/Sparc.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/WebAssembly.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/Basic/Targets/XCore.h
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/test/CodeGen/aix_alloca_align.c

Index: clang/test/CodeGen/aix_alloca_align.c
===
--- /dev/null
+++ clang/test/CodeGen/aix_alloca_align.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple=powerpc-ibm-aix-xcoff -S -emit-llvm < %s | \
+// RUN: FileCheck -check-prefix 32BIT %s
+
+// RUN: %clang_cc1 -triple=powerpc64-ibm-aix-xcoff -S -emit-llvm < %s | \
+// RUN: FileCheck -check-prefix 64BIT %s
+
+typedef long unsigned int size_t;
+extern void *alloca(size_t __size) __attribute__((__nothrow__));
+
+void foo() {
+  char *ptr1 = (char *)alloca(sizeof(char) * 9);
+  int *ptr2 = (int *)alloca(sizeof(int) * 9);
+  double *ptr3 = (double *)alloca(sizeof(double) * 9);
+}
+
+// 32BIT: %0 = alloca i8, i32 9, align 16
+// 32BIT: %1 = alloca i8, i32 36, align 16
+// 32BIT: %3 = alloca i8, i32 72, align 16
+
+// 64BIT: %0 = alloca i8, i64 9, align 16
+// 64BIT: %1 = alloca i8, i64 36, align 16
+// 64BIT: %3 = alloca i8, i64 72, align 16
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -882,7 +882,7 @@
 
   // Define __BIGGEST_ALIGNMENT__ to be compatible with gcc.
   Builder.defineMacro("__BIGGEST_ALIGNMENT__",
-  Twine(TI.getSuitableAlign() / TI.getCharWidth()) );
+  Twine(TI.getGuaranteedAlign() / TI.getCharWidth()));
 
   if (!LangOpts.CharIsSigned)
 Builder.defineMacro("__CHAR_UNSIGNED__");
Index: clang/lib/Basic/Targets/XCore.h
===
--- clang/lib/Basic/Targets/XCore.h
+++ clang/lib/Basic/Targets/XCore.h
@@ -29,7 +29,7 @@
   : TargetInfo(Triple) {
 NoAsmVariants = true;
 LongLongAlign = 32;
-SuitableAlign = 32;
+GuaranteedAlign = SuitableAlign = 32;
 DoubleAlign = LongDoubleAlign = 32;
 SizeType = UnsignedInt;
 PtrDiffType = SignedInt;
Index: clang/lib/Basic/Targets/X86.h
===
--- clang/lib/Basic/Targets/X86.h
+++ clang/lib/Basic/Targets/X86.h
@@ -381,7 +381,7 @@
 DoubleAlign = LongLongAlign = 32;
 LongDoubleWidth = 96;
 LongDoubleAlign = 32;
-SuitableAlign = 128;
+GuaranteedAlign = SuitableAlign = 128;
 resetDataLayout(Triple.isOSBinFormatMachO() ?
 "e-m:o-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-"
 "f80:32-n8:16:32-S128" :
@@ -481,7 +481,7 @@
   : DarwinTargetInfo(Triple, Opts) {
 LongDoubleWidth = 128;
 LongDoubleAlign = 128;
-SuitableAlign = 128;
+GuaranteedAlign = SuitableAlign = 128;
 MaxVectorAlign = 256;
 // The watchOS simulator uses the builtin bool type for Objective-C.
 llvm::Triple T = llvm::Triple(Triple);
@@ -655,7 +655,7 @@
 LongDoubleAlign = 128;
 LargeArrayMinWidth = 128;
 LargeArrayAlign = 128;
-SuitableAlign = 128;
+GuaranteedAlign = SuitableAlign = 128;
 SizeType = IsX32 ? UnsignedInt : UnsignedLong;
 PtrDiffType = IsX32 ? SignedInt : SignedLong;
 IntPtrType = IsX32 ? SignedInt : SignedLong;
@@ -893,7 +893,7 @@
 public:
   AndroidX86_32TargetInfo(const llvm::Triple