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

Wed Sep 23 16:41:34 PDT 2020

efriedma added inline comments.

================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4539
     CharUnits CCAlign = getParamTypeAlignment(Ty);
     CharUnits TyAlign = getContext().getTypeAlignInChars(Ty);

----------------
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?

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

https://reviews.llvm.org/D86790