[PATCH] D60748: Fix i386 struct and union parameter alignment

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

I don't think this was correct (where by "correct", there, I mean "what GCC does", as this patch is intended to match GCC behavior).

I think this change may well break more cases than it fixes, so IMO, this should be reverted, until it's implemented properly.

Consider one example:

  #include <immintrin.h>
  
  typedef __attribute__((aligned(16))) int alignedint;
  
  struct __attribute__((aligned(64))) X {
      int x;
  //    alignedint y;
  //    __m128 y;
  };
  void g(int x, struct X);
  
  _Static_assert(_Alignof(struct X) == 64);
  
  struct X gx;
  
  void f() {
      g(1, gx);
  }

Note that when compiling this as is GCC does _not_ align X when calling g(). But, as of this change, now clang does. If you uncomment either the __m128 or alignedint lines, and now GCC aligns to 64 bytes too.

This is because GCC's algorithm is a whole lot more complex than what you've implemented. See its function ix86_function_arg_boundary.

The way I interpret GCC, it's doing effectively the following:
StackAlignmentForType(T):

1. If T's alignment is < 16 bytes, return 4.
2. If T is a struct/union/array type, then:
  - recursively call StackAlignmentForType() on each member's type (note -- this ignores any attribute((aligned(N))) directly on the //fields// of a struct, but not those that appear on typedefs, or the underlying types).
  - If all of those calls return alignments < 16, then return 4.
3. Otherwise, return the alignment of T.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60748