Page MenuHomePhabricator

Handle x86 feature specific alignment of __builtin_alloca.
AcceptedPublic

Authored by trixirt on Oct 25 2017, 5:50 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

trixirt created this revision.Oct 25 2017, 5:50 PM
efriedma edited edge metadata.Oct 25 2017, 6:30 PM

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 added inline comments.Oct 26 2017, 12:33 PM
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.

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 updated this revision to Diff 120627.Oct 27 2017, 9:10 AM

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

efriedma added inline comments.Oct 27 2017, 12:01 PM
test/CodeGen/builtins-alloca.c
8

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

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 updated this revision to Diff 120869.Oct 30 2017, 12:31 PM

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 accepted this revision.Oct 30 2017, 12:43 PM

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

This revision is now accepted and ready to land.Oct 30 2017, 12:43 PM
srhines edited edge metadata.Oct 30 2017, 12:44 PM

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.

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