LiuChen3 marked 3 inline comments as done.
LiuChen3 added a comment.
In D60748#1931440 <https://reviews.llvm.org/D60748#1931440>, @jyknight wrote:
> 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.
I think at least the initial patch is correct.
================
Comment at: clang/include/clang/Basic/LangOptions.def:353
+VALUE_LANGOPT(AlignPassingAggregate, 1, 0, "Compatible with gcc default
passing struct and union (x86 only).")
+
----------------
rnk wrote:
> If only codegen needs to know, a CodeGenOption would be better.
The backend does not need this option information.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1556
+
+ for (RecordDecl::field_iterator i = RD->field_begin(), e =
RD->field_end();
+ i != e; ++i) {
----------------
rnk wrote:
> Any time you crack open a record to look at the fields, the code is probably
> wrong the first time you write it. :( In this case, I suspect you are not
> looking at base classes. Consider:
> ```
> struct A {
> MyAlignedType Field;
> };
> struct B : A {};
> void passbyval(B o);
> ```
I'm not sure if I understand what you mean.
```
typedef __attribute__((aligned(16))) int alignedint16;
typedef __attribute__((aligned(64))) int alignedint64;
struct __attribute__((aligned(64))) X2 {
struct __attribute__((aligned(32))) {
int a1;
alignedint16 a2;
} a;
int b;
};
struct B : X2{};
void test(B b)
{
std::cout << b.a.a2 << std::endl;
}
```
This can pass.
================
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.
+ //
----------------
jyknight wrote:
> 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.
Sorry I forget to change it.
================
Comment at: clang/test/CodeGen/x86_32-align-linux.cpp:9
+
+class __attribute__((aligned(64))) X1 {
+ class __attribute__((aligned(32))) {
----------------
jyknight wrote:
> 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?
Sorry that I don't know much about front-end tests. I thought class, struct and
union all need to be tested.
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
