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



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

Reply via email to