Handle x86 feature specific alignment of __builtin_alloca.

[trixirt]

Summary

Handle x86 cases of avx and avx512 of alloca.
gcc aligns these to respectively 32 and 64 bytes so the feature's simd memory access instructions are properly aligned.
This extends the earlier change
https://reviews.llvm.org/D24378

[efriedma]

gcc aligns these to respectively 32 and 64 bytes so the feature's simd memory access instructions are properly aligned.

I... wouldn't have thought people use alloca enough to make it worth adding a weird special case here? But given gcc does it, fine.

lib/Basic/Targets/X86.cpp

750

For clarity, could you delete all the lines which set SuitableAlign in X86.h?

[trixirt]

lib/Basic/Targets/X86.cpp

750

Doing that doesn't effect the test results, but i don't think that is really the best way to go.
Not all the x86 have the suitable alignment as 128, see x86_32 android with 32 bit . this seems like a mistake but i have no way of telling and i realize my change will make that 128 bit which also may be wrong. A deeper problem i am avoiding is connection between the suitable alignment and the many settings of the data layout string. It would seem that the data layout string would also change but that would mean (i think) doing a refactor to switch from a static string to a programatically generated string. If folks want that, i don't mind doing it as a followup.

[efriedma]

Not all the x86 have the suitable alignment as 128, see x86_32 android with 32 bit . this seems like a mistake but i have no way of telling

This is a bug; 32 bits clearly isn't enough alignment on x86. I've verified this gcc uses the same alignment on Android and non-Android targets.

A deeper problem i am avoiding is connection between the suitable alignment and the many settings of the data layout string.

I don't think this has any impact on the data layout? There's a very specific set of alignment values included in the data layout string, and this isn't on the list.

[trixirt]

Better handling of android x86_32
Change the SuitableAlign value only if the new value is bigger than the existing value

[efriedma]

test/CodeGen/builtins-alloca.c

8

This RUN line doesn't make any sense; Android requires SSE2.

[trixirt]

As android 32 target is in question, adding reviewers that added the target and reviewed the addition
I believe android's SuitableAlign should be 128 like the other x86's

test/CodeGen/builtins-alloca.c

8

The android 32 SuitableAlign is set to 32. I agree it doesn't make sense and is likely a mistake. The change came in here
https://reviews.llvm.org/D8357

[trixirt]

Make SuitbleAlign 128 for all x86 by moving the setting to the base X86TargetInfo class
This only changes 32 bit android's value of 32

[efriedma]

LGTM. (But maybe wait a couple days before you merge to see if anyone from the Android side wants to comment.)

[srhines]

In D39313#910071, @trixirt wrote:

As android 32 target is in question, adding reviewers that added the target and reviewed the addition
I believe android's SuitableAlign should be 128 like the other x86's

Can I get some clarification about what SuitableAlign actually requires? Android's malloc will guarantee 8-byte alignment for types <= 8 bytes in size. Every larger request will be 16-byte aligned.

[efriedma]

SuitableAlign controls two things: the value of the predefined macro __BIGGEST_ALIGNMENT__, and the alignment of memory returned by __builtin_alloca. It's not related to malloc().

There is a sort of related value, NewAlign, which is the alignment of the global "operator new" (which gets its memory from malloc() on most systems). This is used for various purposes related to operator new: it sets __STDCPP_DEFAULT_NEW_ALIGNMENT__, it controls whether we use the regular operator new for a class with alignment markings (as opposed to the new C++17 entry points which take alignment as an argument), and is used for a few optimizations. On most systems, we compute a default based on the alignment of "long long" and "long double".

include/clang/Basic/TargetInfo.h

protected:
   unsigned char IntWidth, IntAlign;
   unsigned char HalfWidth, HalfAlign;
   unsigned char FloatWidth, FloatAlign;
   unsigned char DoubleWidth, DoubleAlign;
   unsigned char LongDoubleWidth, LongDoubleAlign, Float128Align;
   unsigned char LargeArrayMinWidth, LargeArrayAlign;
   unsigned char LongWidth, LongAlign;
   unsigned char LongLongWidth, LongLongAlign;
-  unsigned char SuitableAlign;
+  unsigned short SuitableAlign;
   unsigned char DefaultAlignForAttributeAligned;
   unsigned char MinGlobalAlign;
   unsigned char MaxAtomicPromoteWidth, MaxAtomicInlineWidth;
   unsigned short MaxVectorAlign;
   unsigned short MaxTLSAlign;
   unsigned short SimdDefaultAlign;
   unsigned short NewAlign;
   std::unique_ptr<llvm::DataLayout> DataLayout;

lib/Basic/Targets/X86.cpp

   // matches the selected sse level.
   if ((FPMath == FP_SSE && SSELevel < SSE1) ||
       (FPMath == FP_387 && SSELevel >= SSE1)) {
     Diags.Report(diag::err_target_unsupported_fpmath)
         << (FPMath == FP_SSE ? "sse" : "387");
     return false;
   }
 
-  SimdDefaultAlign =
+  SuitableAlign = SimdDefaultAlign =
       hasFeature("avx512f") ? 512 : hasFeature("avx") ? 256 : 128;
   return true;
 }
 
 /// X86TargetInfo::getTargetDefines - Return the set of the X86-specific macro
 /// definitions for this particular subtarget.
 void X86TargetInfo::getTargetDefines(const LangOptions &Opts,
                                      MacroBuilder &Builder) const {

test/CodeGen/builtins-alloca.c

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | \
+// RUN:   FileCheck %s -check-prefix=CHECK_X86_64
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -target-feature +avx | \
+// RUN:   FileCheck %s -check-prefix=CHECK_X86_64_AVX
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -target-feature +avx512f | \
+// RUN:   FileCheck %s -check-prefix=CHECK_X86_64_AVX512
+
+
+char *c1; 
+// CHECK_X86_64: %1 = alloca i8, i64 1, align 16
+// CHECK_X86_64_AVX: %1 = alloca i8, i64 1, align 32
+// CHECK_X86_64_AVX512: %1 = alloca i8, i64 1, align 64
+void f1() {
+    c1 = __builtin_alloca(1);
+}