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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits