[PATCH] D86790: [FE] Use preferred alignment instead of ABI alignment for complete object when applicable

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4539
     CharUnits CCAlign = getParamTypeAlignment(Ty);
     CharUnits TyAlign = getContext().getTypeAlignInChars(Ty);
 
----------------
efriedma wrote:
> jyknight wrote:
> > Xiangling_L wrote:
> > > jasonliu wrote:
> > > > Question: 
> > > > It looks like getNaturalAlignIndirect and getTypeAlignInChars here are all returning ABI alignment.
> > > > But according to the comments, we should use a preferred alignment when it's a complete object. Isn't this complete object? Or I'm missing something?
> > > @jyknight Could you shine a light on this? Personally, I would agree that we have complete objects here, so preferred alignment should be used. And if that is true, changes should be applied on all other target within this file?
> > This code specifies that aggregate parameters are passed on the stack aligned to CCAlign (4 or 8), even when the aggregate _requires_ higher alignment. E.g. `struct X { int a, b; } __attribute__((aligned(8)));` would only be passed on the stack with 4-byte alignment for powerpc-aix. The callee needs the object to have the correct 8-byte alignment, so it must copy the parameter to an 8-byte aligned temporary object (this is what "Realign" asks for).
> > 
> > We don't want that overhead for optional (preferred) alignment. It wouldn't be wrong, but would degrade performance and isn't needed. So, no, this shouldn't use preferred alignment.
> Are you sure we can get away with skipping the realignment? Consider something like the following:
> 
> ```
> struct A { double x; };
> void f(int x, struct A a) { _Static_assert(_Alignof(a) == 8); }
> ```
> 
> _Alignof says the alignment is 8, but the IR says "align 4".  Is that safe?
No. That's not OK, that's definitely a bug -- but not a bug _here_. The bug is that alignof(expression) is returning the wrong answer! What I find interesting is that it doesn't appear to affect codegen -- we seem to correctly generate loads/stores in IR using align 4.

Some more brokenness along the same lines:
```
clang -fsyntax-only -target powerpc-aix -xc++ - <<EOF
struct A { double x; };
static_assert(alignof(A) == 4, ""); // correct
static_assert(__alignof__(A) == 8, ""); // correct
void f(A* a) {static_assert(alignof(*a) == 4, ""); } // correct
void f(A& a) {static_assert(alignof(a) == 8, ""); } // incorrect
EOF
```

I note that this is not an AIX-specific bug -- we have the same bug on i386:
```
clang -fsyntax-only -target i386-linux-gnu -xc++ - <<EOF
static_assert(alignof(double) == 4, "");
static_assert(__alignof__(double) == 8, "");
void f(double* a) {static_assert(alignof(*a) == 4, ""); }
void f(double& a) {static_assert(alignof(a) == 8, ""); }
EOF
```



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

https://reviews.llvm.org/D86790