jyknight added a comment.
Since the ABI this is trying to match is not documented literally anywhere, I
think we need to have some confidence that what this implements is actually the
same as what GCC does. While I wrote up what I think the algorithm is, without
some sort of script to allow testing it against a bunch of examples, I wouldn't
say I'm confident of its correctness.
I'm not sure if you can reverse-engineer what the alignment must have been from
the assembly output, or from some debug flags. Or if maybe doing something
silly like modifying the source to insert a printf would be the best method to
test this.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1542-1544
+ // i386 System V ABI 2.1: Structures and unions assume the alignment of
their
+ // most strictly aligned component.
+ //
----------------
This comment isn't useful. While it may be what the System V ABI document says,
that's clearly incorreect, and is not what the code is or should be doing.
Please document what is actually implemented, instead.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1559
+ if (const auto *AT = QT->getAsArrayTypeUnsafe())
+ TempAlignment = getContext().getTypeAlign(AT->getElementType()) /
8;
+ else // recursively to get each type's alignment
----------------
Also needs to call getTypeStackAlignInBytes?
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1567
+ if (MaxAlignment >= 16)
+ return std::max(MaxAlignment, Align);
+ else // return 4 when all the type alignments less than 16 bytes
----------------
I think this is wrong and that it should only return Align. The computation of
the alignment of the elements is only to see if their alignment is >= 16.
If the alignment of the elements' types is >= 16, but the alignment of the
structure is less than the alignment of one of its elements (e.g. due to
`__attribute__ packed`), we should return the alignment of the structure.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1570
+ return 4;
+ } else if (Align < 16)
+ return MinABIStackAlignInBytes;
----------------
If I understood GCC's algorithm correctly, I think this needs to come first?
================
Comment at: clang/test/CodeGen/x86_32-align-linux.cpp:9
+
+class __attribute__((aligned(64))) X1 {
+ class __attribute__((aligned(32))) {
----------------
Confused me that this was a different X1 than in the test-case above. I'm not
sure why the tests need to be duplicated here in a .cpp file in the first place?
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
