[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I submitted a patch with the changes at D85191 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1778
-std::pair FieldInfo =
-  Context.getTypeInfoInChars(D->getType());
-EffectiveFieldSize = FieldSize = FieldInfo.first;

ebevhan wrote:
> It seems that in factoring out this to setDeclInfo, it was changed from using 
> getTypeInfoInChars to using getTypeInfo+toCharUnitsFromBits. This is causing 
> some downstream issues for us with non-bytesize-multiple types.
> 
> Changing setDeclInfo to use the original function instead seems to work 
> without issues. Would it be acceptable to change this?
In general, we want to avoid representing type size/alignment in bits where it 
isn't necessary; refactoring along those lines is welcome.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1778
-std::pair FieldInfo =
-  Context.getTypeInfoInChars(D->getType());
-EffectiveFieldSize = FieldSize = FieldInfo.first;

It seems that in factoring out this to setDeclInfo, it was changed from using 
getTypeInfoInChars to using getTypeInfo+toCharUnitsFromBits. This is causing 
some downstream issues for us with non-bytesize-multiple types.

Changing setDeclInfo to use the original function instead seems to work without 
issues. Would it be acceptable to change this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG05ad8e942996: [AIX] Implement AIX special alignment rule 
about double/long double (authored by Xiangling_L).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
  clang/test/Layout/aix-Wpacked-no-diagnostics.cpp
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-pack-attr-on-base.cpp
  clang/test/Layout/aix-power-alignment-typedef-2.cpp
  clang/test/Layout/aix-power-alignment-typedef.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 | 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 279991.
Xiangling_L added a comment.

- Simplified the test command line;
- Split the `typedef` related tests into two to address the LIT testcase 
failure on windows platform;


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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
  clang/test/Layout/aix-Wpacked-no-diagnostics.cpp
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-pack-attr-on-base.cpp
  clang/test/Layout/aix-power-alignment-typedef-2.cpp
  clang/test/Layout/aix-power-alignment-typedef.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Jason Liu via Phabricator via cfe-commits]

jasonliu added a comment.

Thanks. No further comments from me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 279930.
Xiangling_L marked 3 inline comments as done.
Xiangling_L added a comment.

Add one more testcase;
Addressed other comments;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
  clang/test/Layout/aix-Wpacked-no-diagnostics.cpp
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-pack-attr-on-base.cpp
  clang/test/Layout/aix-power-alignment-typedef.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 13 inline comments as done.
Xiangling_L added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2418
+
   if (!Target->allowsLargerPreferedTypeAlignment())
 return ABIAlign;

jasonliu wrote:
> Should this if statement go above the `if (const auto *RT = 
> T->getAs()) ` ?
> When Target does not allow larger prefered type alignment, then we should 
> return ABIAlign immediately without going through the RecordType query?
Agree. I will update this.



Comment at: clang/test/Layout/aix-double-struct-member.cpp:2
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s

jasonliu wrote:
> You are not using ` < %s` here. So `-x c++` is redundant?
Yeah, thanks, I will remove it.



Comment at: clang/test/Layout/aix-no-unique-address-with-double.cpp:138
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=4]
+
+int a = sizeof(Empty);

jasonliu wrote:
> I think this case is interesting and may worth adding too:
> ```
> struct F {
>   [[no_unique_address]] Empty emp, emp2;
>   double d;
> };
> ```
Sure.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2418
+
   if (!Target->allowsLargerPreferedTypeAlignment())
 return ABIAlign;

Should this if statement go above the `if (const auto *RT = 
T->getAs()) ` ?
When Target does not allow larger prefered type alignment, then we should 
return ABIAlign immediately without going through the RecordType query?



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1234
+
+  bool DefaultsToAIXPowerAlignment =
+  Context.getTargetInfo().defaultsToAIXPowerAlignment();

Add const?



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1801
+
+  bool DefaultsToAIXPowerAlignment =
+  Context.getTargetInfo().defaultsToAIXPowerAlignment();

const ?



Comment at: clang/lib/Basic/Targets/PPC.h:425
+} else if (Triple.isOSAIX()) {
+  SuitableAlign = 64;
+  LongDoubleWidth = 64;

SuitableAlign is set in line 412 as well. Please consider combining the two 
`if` statements if grouping things together makes code easier to parse.



Comment at: clang/test/Layout/aix-double-struct-member.cpp:2
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s

You are not using ` < %s` here. So `-x c++` is redundant?



Comment at: clang/test/Layout/aix-double-struct-member.cpp:305
+struct A { double x; };
+struct B : A {} b;
+

`b` is not necessary when you take the `sizeof` of B below?



Comment at: clang/test/Layout/aix-no-unique-address-with-double.cpp:138
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=4]
+
+int a = sizeof(Empty);

I think this case is interesting and may worth adding too:
```
struct F {
  [[no_unique_address]] Empty emp, emp2;
  double d;
};
```



Comment at: clang/test/Layout/aix-power-alignment-typedef.cpp:12
+  double x;
+} c;
+typedef struct C __attribute__((__aligned__(2))) CC;

remove `c` ?



Comment at: clang/test/Layout/aix-power-alignment-typedef.cpp:25
+  Dbl x;
+} a;
+

remove `a`?



Comment at: clang/test/Layout/aix-power-alignment-typedef.cpp:43
+  char x;
+} u;
+

remove `u`? 


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

I've done another pass over the code (but did not get through the tests). I 
have no comments about the code at this time. My understanding is that 
@jasonliu will be doing another pass over this patch, so he can approve while 
I'm away on vacation.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added inline comments.



Comment at: clang/test/Layout/aix-Wpacked-no-diagnostics.cpp:15
+
+int a = sizeof(QQ);

hubert.reinterpretcast wrote:
> Is there a reason to drop the `FileCheck` checking for the layout?
I dropped the `FileCheck` because the layout of `QQ` and `Q` are fairly simple 
and wanted the test focus more on `no-diagnostics` side. But I can add it back 
if that helps.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 277417.
Xiangling_L added a comment.

Removed unused var;


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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
  clang/test/Layout/aix-Wpacked-no-diagnostics.cpp
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-pack-attr-on-base.cpp
  clang/test/Layout/aix-power-alignment-typedef.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   struct test2::A (virtual base)
+// CHECK32-NEXT: 12 | long long l1
+// CHECK32-NEXT:| 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1244
+if (!Base->Class->isEmpty() && !HandledFirstNonOverlappingEmptyField) {
+  IsFirstNonEmptyBase = true;
+  // By handling a base class that is not empty, we're handling the

`IsFirstNonEmptyBase` can be removed. It is set, but not used.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 276991.
Xiangling_L marked 6 inline comments as done.
Xiangling_L added a comment.

Set `Handled...` = true for non-AIX power alignment;
Addressed other comments;


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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
  clang/test/Layout/aix-Wpacked-no-diagnostics.cpp
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-pack-attr-on-base.cpp
  clang/test/Layout/aix-power-alignment-typedef.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   struct 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1246
+
+// AIX `power` alignment does not apply the preferred alignment for
+// non-union classes if the source of the alignment (the current base in

Move the comment to above the previous `if` and make the following `if` the 
`else` of the previous `if`.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796
+  bool FoundFirstNonOverlappingEmptyFieldToHandle =
+  DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() &&
+  !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField;

Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > Xiangling_L wrote:
> > > hubert.reinterpretcast wrote:
> > > > The condition is still more complex than I think it should be.
> > > > 
> > > > If we have found a "first" other-than-overlapping-empty-field, then we 
> > > > should set `HandledFirstNonOverlappingEmptyField` to `true` for 
> > > > non-union cases.
> > > > 
> > > > If `HandledFirstNonOverlappingEmptyField` being `false` is not enough 
> > > > for `FieldOffset == CharUnits::Zero()` to be true, then I think the 
> > > > correction would be to set `HandledFirstNonOverlappingEmptyField` in 
> > > > more places.
> > > > 
> > > > I would like to remove the check on `FieldOffset == CharUnits::Zero()` 
> > > > from here and instead have an assertion that 
> > > > `!HandledFirstNonOverlappingEmptyField` implies `FieldOffset == 
> > > > CharUnits::Zero()`.
> > > > 
> > > > Also, since we're managing `HandledFirstNonOverlappingEmptyField` in 
> > > > non-AIX cases, we should remove the `DefaultsToAIXPowerAlignment` 
> > > > condition for what is currently named 
> > > > `FoundFirstNonOverlappingEmptyFieldToHandle` (adjusting uses of it as 
> > > > necessary) and rename `FoundFirstNonOverlappingEmptyFieldToHandle` to 
> > > > `FoundFirstNonOverlappingEmptyField`.
> > > > Also, since we're managing HandledFirstNonOverlappingEmptyField in 
> > > > non-AIX cases, we should remove the DefaultsToAIXPowerAlignment 
> > > > condition for what is currently named 
> > > > FoundFirstNonOverlappingEmptyFieldToHandle 
> > > 
> > > I am not sure if we want to remove the `DefaultsToAIXPowerAlignment` 
> > > condition and bother with maintaining correct status of 
> > > `HandledFirstNonOverlappingEmptyField` for other targets.
> > > 
> > > We are actually claiming `HandledFirstNonOverlappingEmptyField` is an 
> > > auxiliary flag used for AIX only in its definition comments.
> > > 
> > > Besides, if we do want to manage `HandledFirstNonOverlappingEmptyField` 
> > > in non-AIX cases, I noticed that we have to set this flag to `true` 
> > > somewhere for objective-C++ cases. 
> > Okay, the other option I'm open is setting 
> > `HandledFirstNonOverlappingEmptyField` to `true` up front when not dealing 
> > with AIX `power` alignment.
> Thanks, that works too. I will address it in the next commit.
I'm not seeing the change for 
https://reviews.llvm.org/D79719?id=276143#inline-767942?



Comment at: clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp:19
+  char x alignas(4)[8];
+} ;
+

Minor nit: Remove the space before the semicolon.



Comment at: clang/test/Layout/aix-Wpacked-no-diagnostics.cpp:15
+
+int a = sizeof(QQ);

Is there a reason to drop the `FileCheck` checking for the layout?


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 276866.
Xiangling_L marked 9 inline comments as done.
Xiangling_L added a comment.

Fixed a base class related case by adding `IsFirstNonEmpty` flag;
Split the `aix-Wpacked.cpp` testcase into two;
Addressed other comments;


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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
  clang/test/Layout/aix-Wpacked-no-diagnostics.cpp
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-pack-attr-on-base.cpp
  clang/test/Layout/aix-power-alignment-typedef.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1208
+// "first (inherited) member".
+HandledFirstNonOverlappingEmptyField = true;
+

We need some sort of `IsFirstNonEmptyBase` to record that the current base 
qualifies for the alignment upgrade:
```
struct A { double x; };
struct B : A {} b;
```



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1245
+  // space or zero-extent array.
+  if (DefaultsToAIXPowerAlignment && HandledFirstNonOverlappingEmptyField) {
+UnpackedPreferredBaseAlign = UnpackedBaseAlign;

Query `!IsFirstNonEmptyBase` instead of `HandledFirstNonOverlappingEmptyField` 
here.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1263
 
-  // The maximum field alignment overrides base align.
+  assert(!IsUnion && "Unions cannot have base classes.");
+  // AIX `power` alignment does not apply the preferred alignment for non-union

It seems this is a leftover copy of the code that has been moved above?



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1809
+  Context.getTargetInfo().defaultsToAIXPowerAlignment();
+  bool FoundFirstNonOverlappingEmptyField = false;
+  if (DefaultsToAIXPowerAlignment)

The rename I suggested in my previous round of review was in coordination with 
maintaining the value not just for AIX. Since we're only maintaining the value 
for AIX, I prefer the previous name (or 
`FoundFirstNonOverlappingEmptyFieldForAIX`).



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1810
+  bool FoundFirstNonOverlappingEmptyField = false;
+  if (DefaultsToAIXPowerAlignment)
+if (!HandledFirstNonOverlappingEmptyField) {

Please merge the `if` conditions to reduce nesting:
```
  if (DefaultsToAIXPowerAlignment && !HandledFirstNonOverlappingEmptyField) {
```



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1817
+  // We're going to handle the "first member" based on
+  // `FoundNonOverlappingEmptyFieldToHandle` during the current invocation
+  // of this function; record it as handled for future invocations.

Keep this reference to the variable up-to-date with its name.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1820
+  if (FoundFirstNonOverlappingEmptyField)
+// For a union, the current field does not represent all "firsts".
+HandledFirstNonOverlappingEmptyField = !IsUnion;

Change the condition of the `if` here to `!IsOverlappingEmptyField` and move 
the setting of `FoundFirstNonOverlappingEmptyField` to `true` into this `if`.

Move the previous comment and merge it with this one here:
> [ ... ] record it as handled for future invocations (except for unions, 
> because the current field does not represent all "firsts").





Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1928
+  FoundFirstNonOverlappingEmptyField) {
+auto upgradeAlignment = [&](const BuiltinType *BTy) {
+  if (BTy->getKind() == BuiltinType::Double ||

Sorry for not seeing this earlier (I only notice some things when I hide the 
inline comments). I think `performBuiltinTypeAlignmentUpgrade` would read 
better at the call site (and better capture the checking, which is based on the 
kind of built-in type, that is within the lambda).



Comment at: clang/test/Layout/aix-Wpacked.cpp:9
+
+// CHECK-NOT: warning: packed attribute is unnecessary for 'Q' [-Wpacked]
+// CHECK: warning: packed attribute is unnecessary for 'test2::C' [-Wpacked]

Clang diagnostics are normally checked using `-verify` (as opposed to 
`FileCheck`). To use it, I think this needs to be split into the "expecting no 
diagnostics" and the "expecting diagnostics" cases. As it is, I think the 
`CHECK-NOT` has a problem because it checks for plain `'Q'`.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked an inline comment as done.
Xiangling_L added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796
+  bool FoundFirstNonOverlappingEmptyFieldToHandle =
+  DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() &&
+  !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField;

hubert.reinterpretcast wrote:
> Xiangling_L wrote:
> > hubert.reinterpretcast wrote:
> > > The condition is still more complex than I think it should be.
> > > 
> > > If we have found a "first" other-than-overlapping-empty-field, then we 
> > > should set `HandledFirstNonOverlappingEmptyField` to `true` for non-union 
> > > cases.
> > > 
> > > If `HandledFirstNonOverlappingEmptyField` being `false` is not enough for 
> > > `FieldOffset == CharUnits::Zero()` to be true, then I think the 
> > > correction would be to set `HandledFirstNonOverlappingEmptyField` in more 
> > > places.
> > > 
> > > I would like to remove the check on `FieldOffset == CharUnits::Zero()` 
> > > from here and instead have an assertion that 
> > > `!HandledFirstNonOverlappingEmptyField` implies `FieldOffset == 
> > > CharUnits::Zero()`.
> > > 
> > > Also, since we're managing `HandledFirstNonOverlappingEmptyField` in 
> > > non-AIX cases, we should remove the `DefaultsToAIXPowerAlignment` 
> > > condition for what is currently named 
> > > `FoundFirstNonOverlappingEmptyFieldToHandle` (adjusting uses of it as 
> > > necessary) and rename `FoundFirstNonOverlappingEmptyFieldToHandle` to 
> > > `FoundFirstNonOverlappingEmptyField`.
> > > Also, since we're managing HandledFirstNonOverlappingEmptyField in 
> > > non-AIX cases, we should remove the DefaultsToAIXPowerAlignment condition 
> > > for what is currently named FoundFirstNonOverlappingEmptyFieldToHandle 
> > 
> > I am not sure if we want to remove the `DefaultsToAIXPowerAlignment` 
> > condition and bother with maintaining correct status of 
> > `HandledFirstNonOverlappingEmptyField` for other targets.
> > 
> > We are actually claiming `HandledFirstNonOverlappingEmptyField` is an 
> > auxiliary flag used for AIX only in its definition comments.
> > 
> > Besides, if we do want to manage `HandledFirstNonOverlappingEmptyField` in 
> > non-AIX cases, I noticed that we have to set this flag to `true` somewhere 
> > for objective-C++ cases. 
> Okay, the other option I'm open is setting 
> `HandledFirstNonOverlappingEmptyField` to `true` up front when not dealing 
> with AIX `power` alignment.
Thanks, that works too. I will address it in the next commit.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796
+  bool FoundFirstNonOverlappingEmptyFieldToHandle =
+  DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() &&
+  !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField;

Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > The condition is still more complex than I think it should be.
> > 
> > If we have found a "first" other-than-overlapping-empty-field, then we 
> > should set `HandledFirstNonOverlappingEmptyField` to `true` for non-union 
> > cases.
> > 
> > If `HandledFirstNonOverlappingEmptyField` being `false` is not enough for 
> > `FieldOffset == CharUnits::Zero()` to be true, then I think the correction 
> > would be to set `HandledFirstNonOverlappingEmptyField` in more places.
> > 
> > I would like to remove the check on `FieldOffset == CharUnits::Zero()` from 
> > here and instead have an assertion that 
> > `!HandledFirstNonOverlappingEmptyField` implies `FieldOffset == 
> > CharUnits::Zero()`.
> > 
> > Also, since we're managing `HandledFirstNonOverlappingEmptyField` in 
> > non-AIX cases, we should remove the `DefaultsToAIXPowerAlignment` condition 
> > for what is currently named `FoundFirstNonOverlappingEmptyFieldToHandle` 
> > (adjusting uses of it as necessary) and rename 
> > `FoundFirstNonOverlappingEmptyFieldToHandle` to 
> > `FoundFirstNonOverlappingEmptyField`.
> > Also, since we're managing HandledFirstNonOverlappingEmptyField in non-AIX 
> > cases, we should remove the DefaultsToAIXPowerAlignment condition for what 
> > is currently named FoundFirstNonOverlappingEmptyFieldToHandle 
> 
> I am not sure if we want to remove the `DefaultsToAIXPowerAlignment` 
> condition and bother with maintaining correct status of 
> `HandledFirstNonOverlappingEmptyField` for other targets.
> 
> We are actually claiming `HandledFirstNonOverlappingEmptyField` is an 
> auxiliary flag used for AIX only in its definition comments.
> 
> Besides, if we do want to manage `HandledFirstNonOverlappingEmptyField` in 
> non-AIX cases, I noticed that we have to set this flag to `true` somewhere 
> for objective-C++ cases. 
Okay, the other option I'm open is setting 
`HandledFirstNonOverlappingEmptyField` to `true` up front when not dealing with 
AIX `power` alignment.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 276528.
Xiangling_L marked 2 inline comments as done.
Xiangling_L added a comment.

Fixed a -Wpacked related case and added the case to the tests;
Fixed the base class related code issue;
Addressed other comments;


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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-Wpacked.cpp
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-pack-attr-on-base.cpp
  clang/test/Layout/aix-power-alignment-typedef.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   struct test2::A (virtual base)
+// 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 9 inline comments as done.
Xiangling_L added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796
+  bool FoundFirstNonOverlappingEmptyFieldToHandle =
+  DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() &&
+  !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField;

hubert.reinterpretcast wrote:
> The condition is still more complex than I think it should be.
> 
> If we have found a "first" other-than-overlapping-empty-field, then we should 
> set `HandledFirstNonOverlappingEmptyField` to `true` for non-union cases.
> 
> If `HandledFirstNonOverlappingEmptyField` being `false` is not enough for 
> `FieldOffset == CharUnits::Zero()` to be true, then I think the correction 
> would be to set `HandledFirstNonOverlappingEmptyField` in more places.
> 
> I would like to remove the check on `FieldOffset == CharUnits::Zero()` from 
> here and instead have an assertion that 
> `!HandledFirstNonOverlappingEmptyField` implies `FieldOffset == 
> CharUnits::Zero()`.
> 
> Also, since we're managing `HandledFirstNonOverlappingEmptyField` in non-AIX 
> cases, we should remove the `DefaultsToAIXPowerAlignment` condition for what 
> is currently named `FoundFirstNonOverlappingEmptyFieldToHandle` (adjusting 
> uses of it as necessary) and rename 
> `FoundFirstNonOverlappingEmptyFieldToHandle` to 
> `FoundFirstNonOverlappingEmptyField`.
> Also, since we're managing HandledFirstNonOverlappingEmptyField in non-AIX 
> cases, we should remove the DefaultsToAIXPowerAlignment condition for what is 
> currently named FoundFirstNonOverlappingEmptyFieldToHandle 

I am not sure if we want to remove the `DefaultsToAIXPowerAlignment` condition 
and bother with maintaining correct status of 
`HandledFirstNonOverlappingEmptyField` for other targets.

We are actually claiming `HandledFirstNonOverlappingEmptyField` is an auxiliary 
flag used for AIX only in its definition comments.

Besides, if we do want to manage `HandledFirstNonOverlappingEmptyField` in 
non-AIX cases, I noticed that we have to set this flag to `true` somewhere for 
objective-C++ cases. 



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1834
+TypeInfo TI = Context.getTypeInfo(D->getType());
+FieldAlign = Context.toCharUnitsFromBits(TI.Align);
+AlignIsRequired = TI.AlignIsRequired;

hubert.reinterpretcast wrote:
> I guess this works (we have a test for it), but the previous code made a 
> point to use the element type and not the array type (and the comment above 
> says we can't directly query `getTypeInfo` with the array type). 
> @Xiangling_L, can you confirm if the comment is out-of-date and update it?
I am sure `getTypeInfo` can recognize the element type for `IncompleteArray`. I 
will update the comments.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1254
+  // space or zero-extent array.
+  if (DefaultsToAIXPowerAlignment && !getDataSize().isZero()) {
+PreferredBaseAlign = BaseAlign;

This needs to check `HandledFirstNonOverlappingEmptyField`:
```
struct A {
  char x[0];
};
struct B {
  double d;
};
struct C : A, B { char x; } c;
```



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1255
+  if (DefaultsToAIXPowerAlignment && !getDataSize().isZero()) {
+PreferredBaseAlign = BaseAlign;
+  }

Note that `PreferredBaseAlign` is only truly meaningful after this line, and 
related fields, such as `UnpackedPreferredBaseAlign`, require similar 
treatment. With `UnpackedAlignTo` being dependent on a meaningful value of 
`UnpackedPreferredBaseAlign`, it needs an update here as well.

Consider the following source. Noting that `C` and `D` are identical except for 
the packed attribute and yield identical layout properties despite that 
difference, we should be getting a `-Wpacked` warning with `-Wpacked` (but it 
is missing).
```
struct A {
  double d;
};

struct B {
  char x[8];
};

struct [[gnu::packed]] C : B, A { // expected-warning {{packed attribute is 
unnecessary}}
  char x alignas(4)[8];
} c;

struct D : B, A {
  char x alignas(4)[8];
} d;
```
```
*** Dumping AST Record Layout
 0 | struct C
 0 |   struct B (base)
 0 | char [8] x
 8 |   struct A (base)
 8 | double d
16 |   char [8] x
   | [sizeof=24, dsize=24, align=4, preferredalign=4,
   |  nvsize=24, nvalign=4, preferrednvalign=4]
```
```
*** Dumping AST Record Layout
 0 | struct D
 0 |   struct B (base)
 0 | char [8] x
 8 |   struct A (base)
 8 | double d
16 |   char [8] x
   | [sizeof=24, dsize=24, align=4, preferredalign=4,
   |  nvsize=24, nvalign=4, preferrednvalign=4]
```



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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1064
 setSize(getSize() + PtrWidth);
 setDataSize(getSize());
   }

I would suggest setting `HandledFirstNonOverlappingEmptyField` to `true` here 
with an assertion that the current type is not a union.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796
+  bool FoundFirstNonOverlappingEmptyFieldToHandle =
+  DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() &&
+  !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField;

The condition is still more complex than I think it should be.

If we have found a "first" other-than-overlapping-empty-field, then we should 
set `HandledFirstNonOverlappingEmptyField` to `true` for non-union cases.

If `HandledFirstNonOverlappingEmptyField` being `false` is not enough for 
`FieldOffset == CharUnits::Zero()` to be true, then I think the correction 
would be to set `HandledFirstNonOverlappingEmptyField` in more places.

I would like to remove the check on `FieldOffset == CharUnits::Zero()` from 
here and instead have an assertion that `!HandledFirstNonOverlappingEmptyField` 
implies `FieldOffset == CharUnits::Zero()`.

Also, since we're managing `HandledFirstNonOverlappingEmptyField` in non-AIX 
cases, we should remove the `DefaultsToAIXPowerAlignment` condition for what is 
currently named `FoundFirstNonOverlappingEmptyFieldToHandle` (adjusting uses of 
it as necessary) and rename `FoundFirstNonOverlappingEmptyFieldToHandle` to 
`FoundFirstNonOverlappingEmptyField`.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1832
 EffectiveFieldSize = FieldSize = CharUnits::Zero();
 const ArrayType* ATy = Context.getAsArrayType(D->getType());
+TypeInfo TI = Context.getTypeInfo(D->getType());

`ATy` seems to be an unused variable now.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1834
+TypeInfo TI = Context.getTypeInfo(D->getType());
+FieldAlign = Context.toCharUnitsFromBits(TI.Align);
+AlignIsRequired = TI.AlignIsRequired;

I guess this works (we have a test for it), but the previous code made a point 
to use the element type and not the array type (and the comment above says we 
can't directly query `getTypeInfo` with the array type). @Xiangling_L, can you 
confirm if the comment is out-of-date and update it?



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1909
+  if (DefaultsToAIXPowerAlignment && !AlignIsRequired &&
+  (IsUnion || FoundFirstNonOverlappingEmptyFieldToHandle)) {
+auto upgradeAlignment = [&](const BuiltinType *BTy) {

It should now be the case that `FoundFirstNonOverlappingEmptyFieldToHandle` is 
`true` for all union members that are not empty, meaning that the `IsUnion` 
part of the check only serves to admit attempts to handle types that are empty 
(and thus does not have subobjects that would induce an alignment upgrade).


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 276143.
Xiangling_L marked 14 inline comments as done.
Xiangling_L added a comment.

Fixed typedef issue on incomplete array field and add a test for it;
Added a test for where pack attribute on object also apply on base classes;
Addressed other comments;


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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-Wpacked.cpp
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-pack-attr-on-base.cpp
  clang/test/Layout/aix-power-alignment-typedef.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1225
+   Context.getTargetInfo().getTriple().isPS4() ||
+   Context.getTargetInfo().getTriple().isOSAIX()))
+   ? CharUnits::One()

hubert.reinterpretcast wrote:
> Thanks; verified that this is correct with `xlclang++` from IBM XL C/C++ for 
> AIX with:
> ```
> struct A {
>   char x;
> };
> struct B {
>   int x;
> };
> struct __attribute__((__packed__)) C : A, B {} c;
> ```
> 
> Length is 5:
> ```
> [10]m   0x0004  .bss 1  externc
> [11]a4  0x0005   00 CM   RW00
> ```
> 
> @Xiangling_L, I suggest adding a case for this to the tests.
Sure, I will add it.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1910
+
+auto upgradeAlignmentIfQualified = [&](const BuiltinType *BTy) {
+  if (BTy->getKind() == BuiltinType::Double ||

"Qualified" is a term of art in the context of C/C++ types. Please remove 
"IfQualified" from the name. The lambda just needs to be named for what it 
does. When naming it for the conditions where it should be called, "if" (as 
opposed to "assuming") implies that the function checks the condition itself.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1927
+  assert(RD && "Expected non-null RecordDecl.");
+  if (RD) {
+const ASTRecordLayout  = Context.getASTRecordLayout(RD);

Please don't write a check on a variable right after making an assertion on 
what its value should be.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2414
+assert(PreferredAlign >= ABIAlign &&
+   "PreferredAlign is at least as large as ABIAlign.");
+return PreferredAlign;

Minor nit: s/is/should be/;



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1207
+  if (!Base->Class->isEmpty())
+HasNonEmptyBaseClass = true;
+

Just set `HandledFirstNonOverlappingEmptyField` to `true` with a comment before 
the `if`:
By handling a base class that is not empty, we're handling the "first 
(inherited) member".



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1799
   if (D->isBitField()) {
+if (FoundNonOverlappingEmptyFieldToHandle)
+  // For a union, the current field does not represent all "firsts".

Add a comment before the `if`:
```
// We're going to handle the "first member" based on
// `FoundNonOverlappingEmptyFieldToHandle` during the current invocation of
// this function; record it as handled for future invocations.
```

Given the rationale from the comment, move the subject `if` to immediately 
after the determination of `FoundNonOverlappingEmptyFieldToHandle`. That way, 
the setting of `HandledFirstNonOverlappingEmptyField` becomes less complicated 
to track.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1832
 const ArrayType* ATy = Context.getAsArrayType(D->getType());
 FieldAlign = Context.getTypeAlignInChars(ATy->getElementType());
   } else if (const ReferenceType *RT = D->getType()->getAs()) {

It seems that the code to set `AlignIsRequired` is missing from this path.

```
typedef double Dbl __attribute__((__aligned__(2)));
typedef Dbl DblArr[];

union U {
  DblArr fam;
  char x;
};

U u[2];
extern char x[sizeof(u)];
extern char x[4];
```



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1908
+   FoundNonOverlappingEmptyFieldToHandle))) {
+HandledFirstNonOverlappingEmptyField = !IsUnion;
+

I think the `if` condition above is too complicated as a filter for setting 
`HandledFirstNonOverlappingEmptyField`. I would prefer if we don't need to set 
`HandledFirstNonOverlappingEmptyField` here. Please see my other comment about 
`HandledFirstNonOverlappingEmptyField`.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:666
+  /// HasNonEmptyBaseClass - Whether the class has any non-empty class (in the
+  /// sense of (C++11 [meta.unary.prop])) as base.
+  bool HasNonEmptyBaseClass;

Minor nit: s/as base/as a base/;



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:707
+HandledFirstNonOverlappingEmptyField(false),
+   HasNonEmptyBaseClass(false),
+FirstNearlyEmptyVBase(nullptr) {}

Minor nit: Please fix the formatting.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1225
+   Context.getTargetInfo().getTriple().isPS4() ||
+   Context.getTargetInfo().getTriple().isOSAIX()))
+   ? CharUnits::One()

Thanks; verified that this is correct with `xlclang++` from IBM XL C/C++ for 
AIX with:
```
struct A {
  char x;
};
struct B {
  int x;
};
struct __attribute__((__packed__)) C : A, B {} c;
```

Length is 5:
```
[10]m   0x0004  .bss 1  externc
[11]a4  0x0005   00 CM   RW00
```

@Xiangling_L, I suggest adding a case for this to the tests.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1256
+  // space or zero-extent array.
+  if (DefaultsToAIXPowerAlignment && !(getDataSize().isZero() || IsUnion)) {
+PreferredBaseAlign = BaseAlign;

Unions cannot have base classes. Please assert `!IsUnion`.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1260
+
+  // The maximum field alignment overrides base align/preferred base align(AIX
+  // only).

Suggestion:
```
  // The maximum field alignment overrides the base align/(AIX-only) preferred
  // base align.
```



Comment at: clang/test/Layout/aix-power-alignment-typedef.cpp:18
+
+// CHECK-DAG: @_ZN5test11aE = global i32 2, align 4
+

Instead of checking the value of `a`, the alignment can be checked more 
directly from the IR for `cc`.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2409
+const RecordDecl *RD = RT->getDecl();
+return std::max(ABIAlign, static_cast(toBits(
+  getASTRecordLayout(RD).PreferredAlignment)));

hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > Please add a comment regarding the situations where the `ABIAlign` value is 
> > greater than the `PreferredAlignment` value. It may be appropriate to 
> > assert that, absent those cases, the `PreferredAlignment` value is at least 
> > that of `ABIAlign`.
> It does not appear that the maximum of the two values is the correct answer:
> ```
> struct C {
>   double x;
> } c;
> typedef struct C __attribute__((__aligned__(2))) CC;
> 
> CC cc;
> extern char x[__alignof__(cc)];
> extern char x[2]; // this is okay with IBM XL C/C++
> ```
> Please add a comment regarding the situations where the ABIAlign value is 
> greater than the PreferredAlignment value.

I added a `if` condition to guard the situation where `ABIAlign` should be 
returned instead of adding a comment. Please let me know if that is sufficient.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1888
+BTy->getKind() == BuiltinType::LongDouble) {
+  PreferredAlign = CharUnits::fromQuantity(8);
+}

hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > I believe an assertion that `PreferredAlign` was 4 would be appropriate.
> It seems that overriding the value should only be done after additional 
> checks:
> ```
> typedef double __attribute__((__aligned__(2))) Dbl;
> struct A {
>   Dbl x;
> } a;
> extern char x[__alignof__(a)];
> extern char x[2]; // this is okay with IBM XL C/C++
> ```
> 
> I am getting concerned that the logic here overlaps quite a bit with 
> `getPreferredTypeAlign` and refactoring to make the code here more common 
> with `getPreferredTypeAlign` is necessary.
Fixed the typedef related cases with my new changes, and the overlaps were not 
a lot as I expected. So I didn't do any refactoring yet. Please let me know if 
you still think it's necessary to refactor the code somehow.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 275840.
Xiangling_L marked 6 inline comments as done.
Xiangling_L added a comment.

Fixed the `typedef` related issues;
Added more testcases;


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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-Wpacked.cpp
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-power-alignment-typedef.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   struct test2::A (virtual base)
+// CHECK32-NEXT: 12 | long long l1
+// CHECK32-NEXT:| [sizeof=20, 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2409
+const RecordDecl *RD = RT->getDecl();
+return std::max(ABIAlign, static_cast(toBits(
+  getASTRecordLayout(RD).PreferredAlignment)));

Please add a comment regarding the situations where the `ABIAlign` value is 
greater than the `PreferredAlignment` value. It may be appropriate to assert 
that, absent those cases, the `PreferredAlignment` value is at least that of 
`ABIAlign`.



Comment at: clang/lib/AST/ASTContext.cpp:2409
+const RecordDecl *RD = RT->getDecl();
+return std::max(ABIAlign, static_cast(toBits(
+  getASTRecordLayout(RD).PreferredAlignment)));

hubert.reinterpretcast wrote:
> Please add a comment regarding the situations where the `ABIAlign` value is 
> greater than the `PreferredAlignment` value. It may be appropriate to assert 
> that, absent those cases, the `PreferredAlignment` value is at least that of 
> `ABIAlign`.
It does not appear that the maximum of the two values is the correct answer:
```
struct C {
  double x;
} c;
typedef struct C __attribute__((__aligned__(2))) CC;

CC cc;
extern char x[__alignof__(cc)];
extern char x[2]; // this is okay with IBM XL C/C++
```



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1888
+BTy->getKind() == BuiltinType::LongDouble) {
+  PreferredAlign = CharUnits::fromQuantity(8);
+}

hubert.reinterpretcast wrote:
> I believe an assertion that `PreferredAlign` was 4 would be appropriate.
It seems that overriding the value should only be done after additional checks:
```
typedef double __attribute__((__aligned__(2))) Dbl;
struct A {
  Dbl x;
} a;
extern char x[__alignof__(a)];
extern char x[2]; // this is okay with IBM XL C/C++
```

I am getting concerned that the logic here overlaps quite a bit with 
`getPreferredTypeAlign` and refactoring to make the code here more common with 
`getPreferredTypeAlign` is necessary.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1885
+  Context.getBaseElementType(CTy->getElementType())
+  ->getAs())
+if (BTy->getKind() == BuiltinType::Double ||

Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > I believe `castAs` should be expected to succeed here.
> `castAs` is not declared in current context, do we really want to use it by 
> introducing one more header?
`castAs` should be declared:
https://clang.llvm.org/doxygen/classclang_1_1Type.html#a436b8b08ae7f2404b4712d37986194ce

Also, the additional point is that the `if` here should be unnecessary. `BTy` 
should not be null. We should use `castAs` to gain the guarantee (with 
assertions) that `BTy` is not null.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 275728.
Xiangling_L marked 3 inline comments as done.
Xiangling_L added a comment.

Fixed -Wpacked warning issue;
Fixed EmptySubobjects related offset issue;
Fixed zero-extent array in a base class related issue;
Addressed other comments;


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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-Wpacked.cpp
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   struct test2::A (virtual base)
+// CHECK32-NEXT: 12 | long long l1
+// 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 27 inline comments as done.
Xiangling_L added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2424
+  (T->isSpecificBuiltinType(BuiltinType::LongDouble) &&
+   Target->supportsAIXPowerAlignment()))
 // Don't increase the alignment if an alignment attribute was specified on 
a

hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > Xiangling_L wrote:
> > > hubert.reinterpretcast wrote:
> > > > Does `supportsAIXPowerAlignment` express the condition we want to check 
> > > > here? That might be true for an implementation operating with `mac68k` 
> > > > alignment rules.
> > > Yeah, `supportsAIXPowerAlignment` cannot separate the preferred alignment 
> > > of double, long double between `power/natural` and `mac68k` alignment 
> > > rules. But I noticed that currently, AIX target on wyvern or XL don't 
> > > support `mac68k` , so maybe we should leave further changes to the patch 
> > > which is gonna implement `mac68k` alignment rules? The possible solution 
> > > I am thinking is we can add checking if the decl has `AlignMac68kAttr` 
> > > into query to separate things out.
> > > 
> > > Another thing is that once we start supporting mac68k alignment rule(if 
> > > we will), should we also change the ABI align values as well? (e.g. for 
> > > double, it should be 2 instead)
> > If the "base state" is AIX `power` alignment for a platform, I suggest that 
> > the name be `defaultsToAIXPowerAlignment`.
> This last question about the ABI align values is relevant to considerations 
> for `natural` alignment support as well. More generally, the question is 
> whether the "minimum alignment" of the type in a context subject to 
> alternative alignment rules is altered to match said alignment rule. This is 
> observable via the diagnostic associated with C++11 alignment specifiers.
> 
> The existing behaviour of `mac68k` alignment suggests that the "minimum 
> alignment" is context-free.
> 
> ```
> #pragma options align=mac68k
> struct Q {
>   double x alignas(2);  // expected-error {{less than minimum alignment}}
> };
> #pragma options align=reset
> ```
> 
> Compiler Explorer link: https://godbolt.org/z/9NM5_-
Thank you for your explanation.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1885
+  Context.getBaseElementType(CTy->getElementType())
+  ->getAs())
+if (BTy->getKind() == BuiltinType::Double ||

hubert.reinterpretcast wrote:
> I believe `castAs` should be expected to succeed here.
`castAs` is not declared in current context, do we really want to use it by 
introducing one more header?



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1898
+} else if (const RecordType *RT = D->getType()
+  ->getBaseElementTypeUnsafe()
+  ->getAs()) {

hubert.reinterpretcast wrote:
> Is there a reason to use `getBaseElementTypeUnsafe` for this case and 
> `Context.getBaseElementType` for the other ones? Also, it would make sense to 
> factor out the array-type considerations once at the top of the if-else chain 
> instead of doing so in each alternative.
Sorry, I didn't pay attention to the different versions of `getBaseElementType` 
functions here and I believe this part of code came from our old compiler's 
changesets. My understanding would be since type qualifiers are not very 
meaningful in our case and `getBaseElementTypeUnsafe()` is more efficient than 
`getBaseElementType()`, we can use `getBaseElementTypeUnsafe()` all over the 
place instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2424
+  (T->isSpecificBuiltinType(BuiltinType::LongDouble) &&
+   Target->supportsAIXPowerAlignment()))
 // Don't increase the alignment if an alignment attribute was specified on 
a

hubert.reinterpretcast wrote:
> Xiangling_L wrote:
> > hubert.reinterpretcast wrote:
> > > Does `supportsAIXPowerAlignment` express the condition we want to check 
> > > here? That might be true for an implementation operating with `mac68k` 
> > > alignment rules.
> > Yeah, `supportsAIXPowerAlignment` cannot separate the preferred alignment 
> > of double, long double between `power/natural` and `mac68k` alignment 
> > rules. But I noticed that currently, AIX target on wyvern or XL don't 
> > support `mac68k` , so maybe we should leave further changes to the patch 
> > which is gonna implement `mac68k` alignment rules? The possible solution I 
> > am thinking is we can add checking if the decl has `AlignMac68kAttr` into 
> > query to separate things out.
> > 
> > Another thing is that once we start supporting mac68k alignment rule(if we 
> > will), should we also change the ABI align values as well? (e.g. for 
> > double, it should be 2 instead)
> If the "base state" is AIX `power` alignment for a platform, I suggest that 
> the name be `defaultsToAIXPowerAlignment`.
This last question about the ABI align values is relevant to considerations for 
`natural` alignment support as well. More generally, the question is whether 
the "minimum alignment" of the type in a context subject to alternative 
alignment rules is altered to match said alignment rule. This is observable via 
the diagnostic associated with C++11 alignment specifiers.

The existing behaviour of `mac68k` alignment suggests that the "minimum 
alignment" is context-free.

```
#pragma options align=mac68k
struct Q {
  double x alignas(2);  // expected-error {{less than minimum alignment}}
};
#pragma options align=reset
```

Compiler Explorer link: https://godbolt.org/z/9NM5_-


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1908
   // The align if the field is not packed. This is to check if the attribute
   // was unnecessary (-Wpacked).
   CharUnits UnpackedFieldAlign = FieldAlign;

Testing using my build with this patch seems to indicate that tracking 
`FieldAlign` for `UnpackedFieldAlign` does not lead to the desired result.

The `QQ` and `ZZ` cases differ only on the `packed` attribute on `Q`. They are 
observed to have different sizes; however, we get a `-Wpacked` diagnostic 
claiming that the `packed` attribute was unnecessary (and could therefore be 
removed).

```
struct [[gnu::packed]] Q {
  double x [[gnu::aligned(4)]];
};
struct QQ : Q { char x; };

struct Z {
  double x [[gnu::aligned(4)]];
};
struct ZZ : Z { char x; };

extern char qx[sizeof(QQ)];
extern char qx[12];
extern char qz[sizeof(ZZ)];
extern char qz[16];
```

```
:1:24: warning: packed attribute is unnecessary for 'Q' [-Wpacked]
struct [[gnu::packed]] Q {
   ^
1 warning generated.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1235
 
+  // Do not use AIX special alignment if current base is not the first member 
or
+  // the struct is not a union.

hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > Suggestion:
> > ```
> >   // AIX `power` alignment does not apply the preferred alignment for 
> > non-union
> >   // classes if the source of the alignment (the current base in this 
> > context)
> >   // follows introduction of the first member with allocated space.
> > ```
> Adjustment to my suggestion:
> s/first member with allocated space/first subobject with exclusively 
> allocated space/;
Add:
[ ... ] or zero-extent array.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1877
+  CharUnits PreferredAlign = FieldAlign;
+  if (SupportsAIXPowerAlignment && FieldOffset == CharUnits::Zero() &&
+  (IsUnion || FoundNonOverlappingEmptyField)) {

This `if` condition does not currently capture that a zero-extent array in a 
base class renders the base class not empty.

```
struct Z { char zea[0]; };
struct A {
  Z z [[no_unique_address]];
  double d;
};
struct B : Z { double d; };
static_assert(__alignof__(A) == __alignof__(B));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/Layout/aix-double-struct-member.cpp:1
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \

Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > Xiangling_L wrote:
> > > hubert.reinterpretcast wrote:
> > > > I am concerned that none of the tests actually create an instance of 
> > > > the classes under test and check the alignment (or related adjustments) 
> > > > in the IR. That is, we set up the preferred alignment value but don't 
> > > > check that we use it where we should.
> > > > 
> > > > As it is, it seems array new/delete has problems:
> > > > ```
> > > > #include 
> > > > extern "C" void *calloc(decltype(sizeof 0), decltype(sizeof 0));
> > > > extern "C" void free(void *);
> > > > extern "C" int printf(const char *, ...);
> > > > 
> > > > extern void *allocated_ptr;
> > > > extern decltype(sizeof 0) allocated_size;
> > > > struct B {
> > > >   double d;
> > > >   ~B() {}
> > > >   static void *operator new[](decltype(sizeof 0) sz);
> > > >   static void operator delete[](void *p, decltype(sizeof 0) sz);
> > > > };
> > > > B *allocBp();
> > > > 
> > > > #ifdef ALLOCBP
> > > > void *allocated_ptr;
> > > > decltype(sizeof 0) allocated_size;
> > > > void *B::operator new[](decltype(sizeof 0) sz) {
> > > >   void *alloc = calloc(1u, allocated_size = sz);
> > > >   printf("%p: %s\n", alloc, __PRETTY_FUNCTION__);
> > > >   printf("%zu\n", sz);
> > > >   return allocated_ptr = alloc;
> > > > }
> > > > void B::operator delete[](void *p, decltype(sizeof 0) sz) {
> > > >   printf("%p: %s\n", p, __PRETTY_FUNCTION__);
> > > >   printf("%zu\n", sz);
> > > >   assert(sz == allocated_size);
> > > >   assert(p == allocated_ptr);
> > > >   free(p);
> > > > }
> > > > B *allocBp() { return new B[2]; }
> > > > #endif
> > > > 
> > > > #ifdef MAIN
> > > > int main(void) { delete[] allocBp(); }
> > > > #endif
> > > > ```
> > > > 
> > > > The `xlclang++` invocation from XL C/C++ generates padding before the 
> > > > 32-bit `new[]` cookie. I'm not seeing that padding with this patch.
> > > Thank. I will create more practical testcases as you mentioned in your 
> > > concern. And regarding to `padding before the 32-bit new[] cookie` issue, 
> > > I am wondering is that part of `power` alignment rule or what rules do we 
> > > follow to generate this kind of padding?
> > The padding has to do with the alignment. The allocation function returns 
> > 8-byte aligned memory. The 32-bit cookie takes 4 of the first 8 bytes. The 
> > type's preferred alignment is 8, so there are 4 bytes of padding.
> Regarding with checking the alignment where we use them, AFAIK the 
> problematic cases include not only the `cookie padding` issue you mentioned 
> here, but also the alignment of argument type, return type etc.
> 
> So I am wondering does it make sense to have them handled in a separate patch 
> since this is already a big one? We can use this patch to implement the 
> correct value of `__alignof` and `alignof` and use a second patch to handle 
> the places where `we use them where we should`?
> 
Yes, we can scope the patch that way somewhat; however, some cases of "[using 
the `__alignof__` value] where we should" that is missing is within the 
determination of the base and field offsets. We should keep those within the 
scope of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 5 inline comments as done.
Xiangling_L added inline comments.



Comment at: clang/test/Layout/aix-double-struct-member.cpp:1
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \

hubert.reinterpretcast wrote:
> Xiangling_L wrote:
> > hubert.reinterpretcast wrote:
> > > I am concerned that none of the tests actually create an instance of the 
> > > classes under test and check the alignment (or related adjustments) in 
> > > the IR. That is, we set up the preferred alignment value but don't check 
> > > that we use it where we should.
> > > 
> > > As it is, it seems array new/delete has problems:
> > > ```
> > > #include 
> > > extern "C" void *calloc(decltype(sizeof 0), decltype(sizeof 0));
> > > extern "C" void free(void *);
> > > extern "C" int printf(const char *, ...);
> > > 
> > > extern void *allocated_ptr;
> > > extern decltype(sizeof 0) allocated_size;
> > > struct B {
> > >   double d;
> > >   ~B() {}
> > >   static void *operator new[](decltype(sizeof 0) sz);
> > >   static void operator delete[](void *p, decltype(sizeof 0) sz);
> > > };
> > > B *allocBp();
> > > 
> > > #ifdef ALLOCBP
> > > void *allocated_ptr;
> > > decltype(sizeof 0) allocated_size;
> > > void *B::operator new[](decltype(sizeof 0) sz) {
> > >   void *alloc = calloc(1u, allocated_size = sz);
> > >   printf("%p: %s\n", alloc, __PRETTY_FUNCTION__);
> > >   printf("%zu\n", sz);
> > >   return allocated_ptr = alloc;
> > > }
> > > void B::operator delete[](void *p, decltype(sizeof 0) sz) {
> > >   printf("%p: %s\n", p, __PRETTY_FUNCTION__);
> > >   printf("%zu\n", sz);
> > >   assert(sz == allocated_size);
> > >   assert(p == allocated_ptr);
> > >   free(p);
> > > }
> > > B *allocBp() { return new B[2]; }
> > > #endif
> > > 
> > > #ifdef MAIN
> > > int main(void) { delete[] allocBp(); }
> > > #endif
> > > ```
> > > 
> > > The `xlclang++` invocation from XL C/C++ generates padding before the 
> > > 32-bit `new[]` cookie. I'm not seeing that padding with this patch.
> > Thank. I will create more practical testcases as you mentioned in your 
> > concern. And regarding to `padding before the 32-bit new[] cookie` issue, I 
> > am wondering is that part of `power` alignment rule or what rules do we 
> > follow to generate this kind of padding?
> The padding has to do with the alignment. The allocation function returns 
> 8-byte aligned memory. The 32-bit cookie takes 4 of the first 8 bytes. The 
> type's preferred alignment is 8, so there are 4 bytes of padding.
Regarding with checking the alignment where we use them, AFAIK the problematic 
cases include not only the `cookie padding` issue you mentioned here, but also 
the alignment of argument type, return type etc.

So I am wondering does it make sense to have them handled in a separate patch 
since this is already a big one? We can use this patch to implement the correct 
value of `__alignof` and `alignof` and use a second patch to handle the places 
where `we use them where we should`?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1884
+  if (const BuiltinType *BTy =
+  Context.getBaseElementType(CTy->getElementType())
+  ->getAs())

I don't think there's a reason to use `getBaseElementType` (which is used to 
handle arrays) on the element type of a `ComplexType`.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1885
+  Context.getBaseElementType(CTy->getElementType())
+  ->getAs())
+if (BTy->getKind() == BuiltinType::Double ||

I believe `castAs` should be expected to succeed here.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1888
+BTy->getKind() == BuiltinType::LongDouble) {
+  PreferredAlign = CharUnits::fromQuantity(8);
+}

I believe an assertion that `PreferredAlign` was 4 would be appropriate.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1893
+   ->getAs()) {
+  if (BTy->getKind() == BuiltinType::Double ||
+  BTy->getKind() == BuiltinType::LongDouble) {

Use a lambda instead of duplicating the code.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1898
+} else if (const RecordType *RT = D->getType()
+  ->getBaseElementTypeUnsafe()
+  ->getAs()) {

Is there a reason to use `getBaseElementTypeUnsafe` for this case and 
`Context.getBaseElementType` for the other ones? Also, it would make sense to 
factor out the array-type considerations once at the top of the if-else chain 
instead of doing so in each alternative.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1900
+  ->getAs()) {
+  if (const RecordDecl *RD = RT->getDecl()) {
+const ASTRecordLayout  = Context.getASTRecordLayout(RD);

I'd be a bit concerned if this failed. Can we assert that we get a non-null 
pointer back?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1778
+if (FoundNonOverlappingEmptyField)
+  HandledFirstNonOverlappingEmptyField = true;
+

See my other comment for rationale on why 
`HandledFirstNonOverlappingEmptyField` should be set to `!IsUnion` instead of 
`true`.

A comment would be appropriate here:
```
if (FoundNonOverlappingEmptyField) {
  // For a union, the current field does not represent all "firsts".
  HandledFirstNonOverlappingEmptyField = !IsUnion;
}
```



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1879
+  (IsUnion || FoundNonOverlappingEmptyField)) {
+HandledFirstNonOverlappingEmptyField = true;
+

`IsOverlappingEmptyField` is still in play (as seen in further checks below). I 
do not believe it is appropriate to set `HandledFirstNonOverlappingEmptyField` 
to `true` in that case (possible because `IsUnion` overrides what is currently 
called `FoundNonOverlappingEmptyField`).

There are a few ways to address this. For example, 
`HandledFirstNonOverlappingEmptyField` could be set to `!IsUnion` instead for 
to `true`. This is semantically sound, because 
`HandledFirstNonOverlappingEmptyField` is meant to indicate that there would 
not be further "firsts" to consider (and that is not true for unions).

I believe the code should not require user to convince themselves that 
non-sequitur logic washes away, so I would like a fix for this although it 
introduces no functional change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1871
 
+  // AIX ABI has this special rule that in aggregates, the first member of
+  // floating point data type(or aggregate type contains floating point data

Suggestion:
```
  // The AIX `power` alignment rules apply the natural alignment of the
  // "first member" if it is of a floating-point data type (or is an aggregate
  // whose recursively "first" member or element is such a type). The alignment
  // associated with these types for subsequent members use an alignment value
  // where the floating-point data type is considered to have 4-byte alignment.
  //
  // For the purposes of the foregoing: vtable pointers, non-empty base classes,
  // and zero-width bit-fields count as prior members; members of empty class
  // types marked `no_unique_address` are not considered to be prior members.
```

This fixes a number of issues with the comment, including:

  - The meaning of "first member" is unclear and the intended meaning is 
unlikely to be understood from common meanings of the term.
  - The recursive application of the rule was not captured (the relationship is 
not merely "contains").
  - The statement about 4-byte alignment needed to take stricter alignment due 
to other factors into account.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1235
 
+  // Do not use AIX special alignment if current base is not the first member 
or
+  // the struct is not a union.

hubert.reinterpretcast wrote:
> Suggestion:
> ```
>   // AIX `power` alignment does not apply the preferred alignment for 
> non-union
>   // classes if the source of the alignment (the current base in this context)
>   // follows introduction of the first member with allocated space.
> ```
Adjustment to my suggestion:
s/first member with allocated space/first subobject with exclusively allocated 
space/;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/Layout/aix-double-struct-member.cpp:1
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \

Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > I am concerned that none of the tests actually create an instance of the 
> > classes under test and check the alignment (or related adjustments) in the 
> > IR. That is, we set up the preferred alignment value but don't check that 
> > we use it where we should.
> > 
> > As it is, it seems array new/delete has problems:
> > ```
> > #include 
> > extern "C" void *calloc(decltype(sizeof 0), decltype(sizeof 0));
> > extern "C" void free(void *);
> > extern "C" int printf(const char *, ...);
> > 
> > extern void *allocated_ptr;
> > extern decltype(sizeof 0) allocated_size;
> > struct B {
> >   double d;
> >   ~B() {}
> >   static void *operator new[](decltype(sizeof 0) sz);
> >   static void operator delete[](void *p, decltype(sizeof 0) sz);
> > };
> > B *allocBp();
> > 
> > #ifdef ALLOCBP
> > void *allocated_ptr;
> > decltype(sizeof 0) allocated_size;
> > void *B::operator new[](decltype(sizeof 0) sz) {
> >   void *alloc = calloc(1u, allocated_size = sz);
> >   printf("%p: %s\n", alloc, __PRETTY_FUNCTION__);
> >   printf("%zu\n", sz);
> >   return allocated_ptr = alloc;
> > }
> > void B::operator delete[](void *p, decltype(sizeof 0) sz) {
> >   printf("%p: %s\n", p, __PRETTY_FUNCTION__);
> >   printf("%zu\n", sz);
> >   assert(sz == allocated_size);
> >   assert(p == allocated_ptr);
> >   free(p);
> > }
> > B *allocBp() { return new B[2]; }
> > #endif
> > 
> > #ifdef MAIN
> > int main(void) { delete[] allocBp(); }
> > #endif
> > ```
> > 
> > The `xlclang++` invocation from XL C/C++ generates padding before the 
> > 32-bit `new[]` cookie. I'm not seeing that padding with this patch.
> Thank. I will create more practical testcases as you mentioned in your 
> concern. And regarding to `padding before the 32-bit new[] cookie` issue, I 
> am wondering is that part of `power` alignment rule or what rules do we 
> follow to generate this kind of padding?
The padding has to do with the alignment. The allocation function returns 
8-byte aligned memory. The 32-bit cookie takes 4 of the first 8 bytes. The 
type's preferred alignment is 8, so there are 4 bytes of padding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked an inline comment as done.
Xiangling_L added inline comments.



Comment at: clang/test/Layout/aix-double-struct-member.cpp:1
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \

hubert.reinterpretcast wrote:
> I am concerned that none of the tests actually create an instance of the 
> classes under test and check the alignment (or related adjustments) in the 
> IR. That is, we set up the preferred alignment value but don't check that we 
> use it where we should.
> 
> As it is, it seems array new/delete has problems:
> ```
> #include 
> extern "C" void *calloc(decltype(sizeof 0), decltype(sizeof 0));
> extern "C" void free(void *);
> extern "C" int printf(const char *, ...);
> 
> extern void *allocated_ptr;
> extern decltype(sizeof 0) allocated_size;
> struct B {
>   double d;
>   ~B() {}
>   static void *operator new[](decltype(sizeof 0) sz);
>   static void operator delete[](void *p, decltype(sizeof 0) sz);
> };
> B *allocBp();
> 
> #ifdef ALLOCBP
> void *allocated_ptr;
> decltype(sizeof 0) allocated_size;
> void *B::operator new[](decltype(sizeof 0) sz) {
>   void *alloc = calloc(1u, allocated_size = sz);
>   printf("%p: %s\n", alloc, __PRETTY_FUNCTION__);
>   printf("%zu\n", sz);
>   return allocated_ptr = alloc;
> }
> void B::operator delete[](void *p, decltype(sizeof 0) sz) {
>   printf("%p: %s\n", p, __PRETTY_FUNCTION__);
>   printf("%zu\n", sz);
>   assert(sz == allocated_size);
>   assert(p == allocated_ptr);
>   free(p);
> }
> B *allocBp() { return new B[2]; }
> #endif
> 
> #ifdef MAIN
> int main(void) { delete[] allocBp(); }
> #endif
> ```
> 
> The `xlclang++` invocation from XL C/C++ generates padding before the 32-bit 
> `new[]` cookie. I'm not seeing that padding with this patch.
Thank. I will create more practical testcases as you mentioned in your concern. 
And regarding to `padding before the 32-bit new[] cookie` issue, I am wondering 
is that part of `power` alignment rule or what rules do we follow to generate 
this kind of padding?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2416
 
-  // Double and long long should be naturally aligned if possible.
+  // Double, long double (only when the target supports AIX power alignment) 
and
+  // long long should be naturally aligned if possible.

I suggest to clarify the binding of the parenthetical and also to make the 
context that the required alignment is lower than the natural alignment more 
explicit:
```
  // Double (and, for targets supporting AIX `power` alignment, long double) and
  // long long should be naturally aligned (despite requiring less alignment) if
  // possible.
```



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1210
 
+  auto getBaseOrPreferredAlign = [&](CharUnits UnpackedAlign) {
+return (Packed && ((Context.getLangOpts().getClangABICompat() <=

The lambda is currently being named for what is produced based on the identity 
of what gets passed to it and not named for what it does.

This should be named `getPackedBaseAlignFromUnpacked` or similar.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1212
+return (Packed && ((Context.getLangOpts().getClangABICompat() <=
+LangOptions::ClangABI::Ver6) ||
+   Context.getTargetInfo().getTriple().isPS4()))

I suggest not applying "ABI compat" with Clang <= 6 on AIX. There was is no 
Clang <= 6 with AIX support.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1218
+
   // Clang <= 6 incorrectly applied the 'packed' attribute to base classes.
   // Per GCC's documentation, it only applies to non-static data members.

This comment belongs inside the lambda.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1235
 
+  // Do not use AIX special alignment if current base is not the first member 
or
+  // the struct is not a union.

Suggestion:
```
  // AIX `power` alignment does not apply the preferred alignment for non-union
  // classes if the source of the alignment (the current base in this context)
  // follows introduction of the first member with allocated space.
```



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1769
+
+  bool FoundNonOverlappingEmptyField = false;
+  bool SupportsAIXPowerAlignment =

The name is wrong for the value (the value corresponding to this name is 
`!IsOverlappingEmptyField`). I would suggest something like 
`FoundNonOverlappingEmptyFieldToHandle`.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1772
+  Context.getTargetInfo().defaultsToAIXPowerAlignment();
+  if (SupportsAIXPowerAlignment && !HandledFirstNonOverlappingEmptyField &&
+  !IsOverlappingEmptyField)

Move the definition of the variable here and just use the `if` condition as the 
initializer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/Layout/aix-double-struct-member.cpp:2
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck %s

My build with this patch also hits cases where the preferred alignment is 
increased to 8, but the offset of the associated member is not 8-byte aligned:
```
extern "C" int printf(const char *, ...);
struct A0 {};
struct Y : A0 {
  double mem;
};
struct Z : A0 {
  Y y;
};
extern char alignz[__alignof__(Z)];
extern char alignz[8];
int main(void) {
  Z z;
  printf("%td\n",
 _cast(z.y) - _cast(z));
}
```
```
*** Dumping AST Record Layout
 0 | struct Z
 0 |   struct A0 (base) (empty)
 4 |   struct Y y
 4 | struct A0 (base) (empty)
 4 | double mem
   | [sizeof=16, dsize=12, align=4, preferredalign=8,
   |  nvsize=12, nvalign=4, preferrednvalign=8]
```




Comment at: clang/test/Layout/aix-double-struct-member.cpp:237
+
+namespace tes8 {
+// Test how #pragma pack and align attribute interacts with AIX alignment.

Typo: s/tes8/test8/;



Comment at: clang/test/Layout/aix-double-struct-member.cpp:347
+
+} // namespace tes8

Same comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/Layout/aix-double-struct-member.cpp:1
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \

I am concerned that none of the tests actually create an instance of the 
classes under test and check the alignment (or related adjustments) in the IR. 
That is, we set up the preferred alignment value but don't check that we use it 
where we should.

As it is, it seems array new/delete has problems:
```
#include 
extern "C" void *calloc(decltype(sizeof 0), decltype(sizeof 0));
extern "C" void free(void *);
extern "C" int printf(const char *, ...);

extern void *allocated_ptr;
extern decltype(sizeof 0) allocated_size;
struct B {
  double d;
  ~B() {}
  static void *operator new[](decltype(sizeof 0) sz);
  static void operator delete[](void *p, decltype(sizeof 0) sz);
};
B *allocBp();

#ifdef ALLOCBP
void *allocated_ptr;
decltype(sizeof 0) allocated_size;
void *B::operator new[](decltype(sizeof 0) sz) {
  void *alloc = calloc(1u, allocated_size = sz);
  printf("%p: %s\n", alloc, __PRETTY_FUNCTION__);
  printf("%zu\n", sz);
  return allocated_ptr = alloc;
}
void B::operator delete[](void *p, decltype(sizeof 0) sz) {
  printf("%p: %s\n", p, __PRETTY_FUNCTION__);
  printf("%zu\n", sz);
  assert(sz == allocated_size);
  assert(p == allocated_ptr);
  free(p);
}
B *allocBp() { return new B[2]; }
#endif

#ifdef MAIN
int main(void) { delete[] allocBp(); }
#endif
```

The `xlclang++` invocation from XL C/C++ generates padding before the 32-bit 
`new[]` cookie. I'm not seeing that padding with this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 273708.
Xiangling_L marked 2 inline comments as done.
Xiangling_L added a comment.

Corrected the comments;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   struct test2::A (virtual base)
+// CHECK32-NEXT: 12 | long long l1
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=2, preferredalign=2,
+// CHECK32-NEXT:|  nvsize=12, 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/include/clang/AST/RecordLayout.h:75
+  // can be different than Alignment in cases where it is beneficial for
+  // performance or backwards compatibility perserving (e.g. AIX-ABI).
+  CharUnits PreferredAlignment;

I'm still evaluating this; however, I might as well note a minor nit first:
s/perserving/preserving/g;
for all instances within this patch.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Jason Liu via Phabricator via cfe-commits]

jasonliu accepted this revision.
jasonliu added a comment.

LGTM. I have no further comments on this patch.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1881
+  if (isAIXLayout(Context) && FieldOffset == CharUnits::Zero() &&
+  (IsUnion || NonOverlappingEmptyFieldFound)) {
+FirstNonOverlappingEmptyFieldHandled = true;

Xiangling_L wrote:
> jasonliu wrote:
> > Xiangling_L wrote:
> > > jasonliu wrote:
> > > > Maybe it's a naive thought, but is it possible to replace 
> > > > `NonOverlappingEmptyFieldFound` with `IsOverlappingEmptyField && 
> > > > FieldOffsets.size() == 0`?
> > > I don't think these two work the same. `NonOverlappingEmptyFieldFound` 
> > > represents the 1st non-empty and non-overlapping field in the record. 
> > > `IsOverlappingEmptyField && FieldOffsets.size() == 0` represents 
> > > something opposite.
> > You are right. I meant could we replace it with `!(IsOverlappingEmptyField 
> > && FieldOffsets.size() == 0)`?
> I don't think so. The replacement does not work for the case:
> 
> 
> ```
> struct A {
>   int : 0;
>   double d;
> };
> ```
> 
> where __alignof(A) should be 4;
Got it. Thanks!


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 2 inline comments as done.
Xiangling_L added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1881
+  if (isAIXLayout(Context) && FieldOffset == CharUnits::Zero() &&
+  (IsUnion || NonOverlappingEmptyFieldFound)) {
+FirstNonOverlappingEmptyFieldHandled = true;

jasonliu wrote:
> Xiangling_L wrote:
> > jasonliu wrote:
> > > Maybe it's a naive thought, but is it possible to replace 
> > > `NonOverlappingEmptyFieldFound` with `IsOverlappingEmptyField && 
> > > FieldOffsets.size() == 0`?
> > I don't think these two work the same. `NonOverlappingEmptyFieldFound` 
> > represents the 1st non-empty and non-overlapping field in the record. 
> > `IsOverlappingEmptyField && FieldOffsets.size() == 0` represents something 
> > opposite.
> You are right. I meant could we replace it with `!(IsOverlappingEmptyField && 
> FieldOffsets.size() == 0)`?
I don't think so. The replacement does not work for the case:


```
struct A {
  int : 0;
  double d;
};
```

where __alignof(A) should be 4;


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1881
+  if (isAIXLayout(Context) && FieldOffset == CharUnits::Zero() &&
+  (IsUnion || NonOverlappingEmptyFieldFound)) {
+FirstNonOverlappingEmptyFieldHandled = true;

Xiangling_L wrote:
> jasonliu wrote:
> > Maybe it's a naive thought, but is it possible to replace 
> > `NonOverlappingEmptyFieldFound` with `IsOverlappingEmptyField && 
> > FieldOffsets.size() == 0`?
> I don't think these two work the same. `NonOverlappingEmptyFieldFound` 
> represents the 1st non-empty and non-overlapping field in the record. 
> `IsOverlappingEmptyField && FieldOffsets.size() == 0` represents something 
> opposite.
You are right. I meant could we replace it with `!(IsOverlappingEmptyField && 
FieldOffsets.size() == 0)`?


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 272824.
Xiangling_L marked 2 inline comments as done.
Xiangling_L added a comment.

Adjust the function name;
Adjust the comment;


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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   struct test2::A (virtual base)
+// CHECK32-NEXT: 12 | long long l1
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=2, preferredalign=2,
+// CHECK32-NEXT:|  nvsize=12, nvalign=2, 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2424
+  (T->isSpecificBuiltinType(BuiltinType::LongDouble) &&
+   Target->supportsAIXPowerAlignment()))
 // Don't increase the alignment if an alignment attribute was specified on 
a

Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > Does `supportsAIXPowerAlignment` express the condition we want to check 
> > here? That might be true for an implementation operating with `mac68k` 
> > alignment rules.
> Yeah, `supportsAIXPowerAlignment` cannot separate the preferred alignment of 
> double, long double between `power/natural` and `mac68k` alignment rules. But 
> I noticed that currently, AIX target on wyvern or XL don't support `mac68k` , 
> so maybe we should leave further changes to the patch which is gonna 
> implement `mac68k` alignment rules? The possible solution I am thinking is we 
> can add checking if the decl has `AlignMac68kAttr` into query to separate 
> things out.
> 
> Another thing is that once we start supporting mac68k alignment rule(if we 
> will), should we also change the ABI align values as well? (e.g. for double, 
> it should be 2 instead)
If the "base state" is AIX `power` alignment for a platform, I suggest that the 
name be `defaultsToAIXPowerAlignment`.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:660
+  /// When there are OverlappingEmptyFields existing in the aggregate, the
+  /// flag shows if the following first non-overlappingEmptyField has been
+  /// handled, if any.

Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > I suggest to replace (if correct) "non-overlappingEmptyField" with 
> > "non-empty or empty-but-non-overlapping field".
> > 
> Thanks for your suggestion. But I kinda prefer using 
> `NonOverlappingEmptyField`, it is more consistent with 
> `IsOverlappingEmptyField`. And also the equivalent name to 
> `NonOverlappingEmptyField` is `NonOverlappingAndNonEmptyField` which is 
> tedious I think.
I am suggesting a change to the comment and not the name here. If both the 
comment and the name uses the same (possibly confusing) form to express the 
concept, then the comment does not aid comprehension of the code.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 272741.
Xiangling_L marked 24 inline comments as done.
Xiangling_L added a comment.

Addressed comments;
Fixed the ICE problem with array as first member;
Add support for Complex type;


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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   struct test2::A (virtual base)
+// CHECK32-NEXT: 12 | long long l1
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=2, preferredalign=2,
+// 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2424
+  (T->isSpecificBuiltinType(BuiltinType::LongDouble) &&
+   Target->supportsAIXPowerAlignment()))
 // Don't increase the alignment if an alignment attribute was specified on 
a

hubert.reinterpretcast wrote:
> Does `supportsAIXPowerAlignment` express the condition we want to check here? 
> That might be true for an implementation operating with `mac68k` alignment 
> rules.
Yeah, `supportsAIXPowerAlignment` cannot separate the preferred alignment of 
double, long double between `power/natural` and `mac68k` alignment rules. But I 
noticed that currently, AIX target on wyvern or XL don't support `mac68k` , so 
maybe we should leave further changes to the patch which is gonna implement 
`mac68k` alignment rules? The possible solution I am thinking is we can add 
checking if the decl has `AlignMac68kAttr` into query to separate things out.

Another thing is that once we start supporting mac68k alignment rule(if we 
will), should we also change the ABI align values as well? (e.g. for double, it 
should be 2 instead)



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:660
+  /// When there are OverlappingEmptyFields existing in the aggregate, the
+  /// flag shows if the following first non-overlappingEmptyField has been
+  /// handled, if any.

hubert.reinterpretcast wrote:
> I suggest to replace (if correct) "non-overlappingEmptyField" with "non-empty 
> or empty-but-non-overlapping field".
> 
Thanks for your suggestion. But I kinda prefer using 
`NonOverlappingEmptyField`, it is more consistent with 
`IsOverlappingEmptyField`. And also the equivalent name to 
`NonOverlappingEmptyField` is `NonOverlappingAndNonEmptyField` which is tedious 
I think.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1881
+  if (isAIXLayout(Context) && FieldOffset == CharUnits::Zero() &&
+  (IsUnion || NonOverlappingEmptyFieldFound)) {
+FirstNonOverlappingEmptyFieldHandled = true;

jasonliu wrote:
> Maybe it's a naive thought, but is it possible to replace 
> `NonOverlappingEmptyFieldFound` with `IsOverlappingEmptyField && 
> FieldOffsets.size() == 0`?
I don't think these two work the same. `NonOverlappingEmptyFieldFound` 
represents the 1st non-empty and non-overlapping field in the record. 
`IsOverlappingEmptyField && FieldOffsets.size() == 0` represents something 
opposite.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1943
 getDataSize() != CharUnits::Zero())
   FieldOffset = getDataSize().alignTo(FieldAlign);
 else

jasonliu wrote:
> hmm... Are we sure we should use FieldAlign instead of PreferredAlign here?
> Same for the else below.
The way I see the `PreferredAlign` is not the `real alignment` the record uses 
(which I believe the community don't expect us to see it that way either). All 
code here should be attached to `ABI align`. Specially, on AIX, 
`PreferredAlign` is something we use to round up the record size, but it won't 
affect the record layout besides of that. So in other words, we are just 
tracking what `PreferredAlignment` value is here.


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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/Layout/aix-double-struct-member.cpp:1
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \

There's no testing for `_Complex` types.
```
struct A {
  struct B {
long double _Complex d[3];
  } b;
};

extern char x[__alignof__(struct A)];
extern char x[8];
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1887
+  if (BTy->isFloatingPoint()) {
+PreferredAlign = FieldSize;
+  }

I tried `clang -cc1 -triple powerpc-ibm-aix -fsyntax-only` with
:
```
struct A {
  struct B {
double d[3];
  } b;
};

extern char x[__alignof__(struct A)];
extern char x[8];
```

I got an assertion failure:
```
clang: /src/clang/lib/AST/RecordLayoutBuilder.cpp:2078: void 
{anonymous}::ItaniumRecordLayoutBuilder::UpdateAlignment(clang::CharUnits, 
clang::CharUnits, clang::CharUnits): Assertion 
`llvm::isPowerOf2_64(PreferredNewAlignment.getQuantity()) && "Alignment not a 
power of 2"' failed.
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:660
+  /// When there are OverlappingEmptyFields existing in the aggregate, the
+  /// flag shows if the following first non-overlappingEmptyField has been
+  /// handled, if any.

I suggest to replace (if correct) "non-overlappingEmptyField" with "non-empty 
or empty-but-non-overlapping field".




Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:662
+  /// handled, if any.
+  bool FirstNonOverlappingEmptyFieldHandled; 
+

Having "handled" at the front is a better indication (aside from the type) that 
"field" is not the main term. Also, minor nit: there's trailing whitespace at 
the end of the line here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/include/clang/Basic/TargetInfo.h:1389
 
+  /// Whether target supports the special power alignment rules of AIX.
+  virtual bool supportsAIXPowerAlignment() const { return false; }

Minor nit: Use backquotes around the XL option value "power".



Comment at: clang/lib/AST/ASTContext.cpp:2415
 
   // Double and long long should be naturally aligned if possible.
   if (const auto *CT = T->getAs())

Comment should be updated.



Comment at: clang/lib/AST/ASTContext.cpp:2424
+  (T->isSpecificBuiltinType(BuiltinType::LongDouble) &&
+   Target->supportsAIXPowerAlignment()))
 // Don't increase the alignment if an alignment attribute was specified on 
a

Does `supportsAIXPowerAlignment` express the condition we want to check here? 
That might be true for an implementation operating with `mac68k` alignment 
rules.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/include/clang/AST/RecordLayout.h:75
+  // can be different than Alignment in cases where it is beneficial for
+  // performance.
+  CharUnits PreferredAlignment;

nit for comment: I don't think it's related to performance nowadays, and it's 
more for ABI-compat reason. 



Comment at: clang/lib/AST/ASTContext.cpp:2409
+return std::max(
+ABIAlign, (unsigned)toBits(getASTRecordLayout(RD).PreferredAlignment));
+  }

static_cast instead of c cast?



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:826
+static bool isAIXLayout(const ASTContext ) {
+  return Context.getTargetInfo().getTriple().getOS() == llvm::Triple::AIX;
+}

We added supportsAIXPowerAlignment, so let's make use of that.
We could replace every call to isAIXLayout with supportsAIXPowerAlignment. This 
name is more expressive as well. 



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1225
+  CharUnits PreferredAlign =
+  (Packed && ((Context.getLangOpts().getClangABICompat() <=
+   LangOptions::ClangABI::Ver6) ||

Use a lambda for this query would be more preferable if same logic get used 
twice.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1881
+  if (isAIXLayout(Context) && FieldOffset == CharUnits::Zero() &&
+  (IsUnion || NonOverlappingEmptyFieldFound)) {
+FirstNonOverlappingEmptyFieldHandled = true;

Maybe it's a naive thought, but is it possible to replace 
`NonOverlappingEmptyFieldFound` with `IsOverlappingEmptyField && 
FieldOffsets.size() == 0`?



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1943
 getDataSize() != CharUnits::Zero())
   FieldOffset = getDataSize().alignTo(FieldAlign);
 else

hmm... Are we sure we should use FieldAlign instead of PreferredAlign here?
Same for the else below.



Comment at: clang/lib/Basic/Targets/PPC.h:377
 
+if (Triple.isOSAIX()) {
+  LongDoubleWidth = 64;

nit: use "else if" with the above if statement?
If it enters one of the Triples above, it's not necessary to check if it is AIX 
any more. 



Comment at: clang/lib/Basic/Targets/PPC.h:411
 
 if (Triple.getOS() == llvm::Triple::AIX)
   SuitableAlign = 64;

This could get combined with the new if for AIX below. 



Comment at: clang/lib/Basic/Targets/PPC.h:419
 
+if (Triple.isOSAIX()) {
+  LongDoubleWidth = 64;

nit: use "else if" with the above if statement?



Comment at: clang/test/Layout/aix-no-unique-address-with-double.cpp:62
+
+  [[no_unique_address]] Empty emp;
+  A a;

Not an action item, but just an interesting note that, in some cases(like this 
one) power alignment rule could reverse the benefit of having 
[[no_unique_address]] at the first place. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 269553.
Xiangling_L marked an inline comment as done.
Xiangling_L added a comment.

Replace the transient status by a local var;
Clean up the code;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+}; // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   struct test2::A (virtual base)
+// CHECK32-NEXT: 12 | long long l1
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=2, preferredalign=2,
+// 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:666
+FirstNonOverlappingEmptyFieldHandled
+  } FirstNonOverlappingEmptyFieldStatus;
+

Xiangling_L wrote:
> efriedma wrote:
> > Instead of specifically tracking whether you've found an OverlappingEmpty 
> > field, could you just have something like "bool 
> > FoundNonOverlappingEmptyField = false;", and set it to true when you handle 
> > a field that isn't OverlappingEmpty?  I don't think we need to specifically 
> > track whether we've found an OverlappingEmpty field.
> I think you are right. We do not need to specifically track whether we've 
> found an OverlappingEmpty field. But I think we do need an enum to track if 
> the first non-OverlappingEmptyField is handled or not.
> 
> Or the following case is problematic:
> 
> 
> ```
> struct A {
>   int : 0;
>   double d;
> };
> 
> ```
> The `double d` will mistakenly match `FieldOffset == CharUnits::Zero() && 
> D->getFieldIndex() != 0 && !IsOverlappingEmptyField && 
> FoundNonOverlappingEmptyField `, which we shouldn't because it is not the 
> first member of the struct.
> 
> 
> 
Okay, so now there are three states.  But the 
FirstNonOverlappingEmptyFieldFound is transient: on exit from 
ItaniumRecordLayoutBuilder::LayoutField, FirstNonOverlappingEmptyFieldStatus 
never contains FirstNonOverlappingEmptyFieldFound.   I think it would be more 
clear to use a local variable for that.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1884
+  if (isAIXLayout(Context) && FieldOffset == CharUnits::Zero() &&
+  ((D->getFieldIndex() == 0 && !IsOverlappingEmptyField) || IsUnion ||
+   (D->getFieldIndex() != 0 && FirstNonOverlappingEmptyFieldStatus ==

I don't think you need to distinguish the `D->getFieldIndex() == 0` case from 
the `D->getFieldIndex() != 0` case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 269396.
Xiangling_L marked 2 inline comments as done.
Xiangling_L added a comment.

Simplify the code;
Add one more testcase related to [[no_unique_addr]];


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+}; // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   struct test2::A (virtual base)
+// CHECK32-NEXT: 12 | long long l1
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=2, preferredalign=2,
+// 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:666
+FirstNonOverlappingEmptyFieldHandled
+  } FirstNonOverlappingEmptyFieldStatus;
+

efriedma wrote:
> Instead of specifically tracking whether you've found an OverlappingEmpty 
> field, could you just have something like "bool FoundNonOverlappingEmptyField 
> = false;", and set it to true when you handle a field that isn't 
> OverlappingEmpty?  I don't think we need to specifically track whether we've 
> found an OverlappingEmpty field.
I think you are right. We do not need to specifically track whether we've found 
an OverlappingEmpty field. But I think we do need an enum to track if the first 
non-OverlappingEmptyField is handled or not.

Or the following case is problematic:


```
struct A {
  int : 0;
  double d;
};

```
The `double d` will mistakenly match `FieldOffset == CharUnits::Zero() && 
D->getFieldIndex() != 0 && !IsOverlappingEmptyField && 
FoundNonOverlappingEmptyField `, which we shouldn't because it is not the first 
member of the struct.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:666
+FirstNonOverlappingEmptyFieldHandled
+  } FirstNonOverlappingEmptyFieldStatus;
+

Instead of specifically tracking whether you've found an OverlappingEmpty 
field, could you just have something like "bool FoundNonOverlappingEmptyField = 
false;", and set it to true when you handle a field that isn't 
OverlappingEmpty?  I don't think we need to specifically track whether we've 
found an OverlappingEmpty field.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 268840.
Xiangling_L added a comment.

Replace `int` with an more self-explanatory enum;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+}; // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   struct test2::A (virtual base)
+// CHECK32-NEXT: 12 | long long l1
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=2, preferredalign=2,
+// CHECK32-NEXT:|  nvsize=12, nvalign=2, 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:660
+  /// with [[no_unique_attr]] Empty CXXRD invovled in the CXXRD.
+  int IsRealFirstMember;
+

Please explain here what the different values (-1, 0, 1, 2) mean.  Or use an 
enum.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 268624.
Xiangling_L marked 11 inline comments as done.
Xiangling_L added a comment.

Add `PreferredAlignment` and `PreferredNVAlignment` field;
Adjust `-fdump-record-layouts` format for AIX;
Update the testcase formatting;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/RecordLayout.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=4, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4, preferredalign=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8, preferrednvalign=8]
+
+}; // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8, preferredalign=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8, preferrednvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   struct test2::A (virtual base)
+// CHECK32-NEXT: 12 | long long l1
+// 

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2506
 
   if (!Target->allowsLargerPreferedTypeAlignment())
 return ABIAlign;

jyknight wrote:
> I think from here on down is currently X86-specific, even though it's not 
> phrased that way right now.
> 
> It only applies if this target hook doesn't disable it, and if alignof(X) < 
> sizeof(X), for X in {double, long long, unsigned long long}. It would be good 
> to try to determine if there's any other platforms for which those conditions 
> actually exist today, other than x86-32. And then determine if this code 
> block actually _should_ trigger there. (I suspect not.) Then, mark this stuff 
> as truthfully completely-target-specific, instead of pretending otherwise, as 
> we do now.
> 
Thank you for your suggestion. I think currently, there are three targets 
disabling this query, `XCore`, `X86` and `MSP430`. Since @efriedma mentioned we 
tried to avoid OS-specific checks here, I am not sure if mark this 
`completely-target-specific` is a good idea?  Personally, I think a query like 
`allowsLargerPreferedTypeAlignment()` would give more semantic meaning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In the commit message, you refer to the preferred alignemtn as the "true" 
alignment, but that's misleading. As discussed on the mailing list thread, it's 
not true alignment at all.




Comment at: clang/include/clang/AST/RecordLayout.h:74
+  /// The maximum allowed field alignment. This is set by #pragma pack.
+  CharUnits MaxFieldAlignment;
+

efriedma wrote:
> If we have to keep around extra data anyway, probably better to add a field 
> for the preferred alignment, so we can keep the preferred alignment handling 
> together.
+1 to just storing the preferred alignment on the RecordLayout -- it already 
had to compute it to compute the size-adjustment for AIX, anyhow. Then you no 
longer need to store MaxFieldAlignment.



Comment at: clang/lib/AST/ASTContext.cpp:2506
 
   if (!Target->allowsLargerPreferedTypeAlignment())
 return ABIAlign;

I think from here on down is currently X86-specific, even though it's not 
phrased that way right now.

It only applies if this target hook doesn't disable it, and if alignof(X) < 
sizeof(X), for X in {double, long long, unsigned long long}. It would be good 
to try to determine if there's any other platforms for which those conditions 
actually exist today, other than x86-32. And then determine if this code block 
actually _should_ trigger there. (I suspect not.) Then, mark this stuff as 
truthfully completely-target-specific, instead of pretending otherwise, as we 
do now.




Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1944
+
+  uint64_t AlignValue = Context.toBits(Alignment);
+  if (isAIXLayout(Context))

name it RoundSizeTo, to indicate that this is to be used _only_ for aligning 
the size, and isn't actually the alignment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/include/clang/AST/RecordLayout.h:74
+  /// The maximum allowed field alignment. This is set by #pragma pack.
+  CharUnits MaxFieldAlignment;
+

jasonliu wrote:
> efriedma wrote:
> > If we have to keep around extra data anyway, probably better to add a field 
> > for the preferred alignment, so we can keep the preferred alignment 
> > handling together.
> We could get the MaxFieldAlignment in the same way 
> ItaniumRecordLayoutBuilder::InitializeLayout get. Do we need to keep an extra 
> data?
The idea here is that we want to make the division of work more clear: 
RecordLayoutBuilder actually does the relevant computation, and ASTContext just 
returns the result.



Comment at: clang/lib/AST/ASTContext.cpp:2533
+  hasFloatingPointAsFirstMember(RD, MaxFieldAlignment))
+return std::max(ABIAlign, 
(unsigned)toBits(CharUnits::fromQuantity(8)));
+}

jasonliu wrote:
> I guess another thing that worth considering is how we support "pragma 
> align(nature)" in the future. It requires us to get a normal alignment for 
> AIX which means double will be aligned 8 byte in the structure.
> Right now, we always set the double to be aligned 4 byte in PPC.h, but it's 
> not necessary the case if that pragma is in effect. How double should be 
> aligned in a structure is really position dependent (is the structure comes 
> after `pragma align(nature)` or `pragma align(power)`?)  I'm not very clear 
> on how we would handle that with the current architect. 
We convert pragmas like that into attributes in the AST.  See 
Sema::AddAlignmentAttributesForRecord.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2533
+  hasFloatingPointAsFirstMember(RD, MaxFieldAlignment))
+return std::max(ABIAlign, 
(unsigned)toBits(CharUnits::fromQuantity(8)));
+}

I guess another thing that worth considering is how we support "pragma 
align(nature)" in the future. It requires us to get a normal alignment for AIX 
which means double will be aligned 8 byte in the structure.
Right now, we always set the double to be aligned 4 byte in PPC.h, but it's not 
necessary the case if that pragma is in effect. How double should be aligned in 
a structure is really position dependent (is the structure comes after `pragma 
align(nature)` or `pragma align(power)`?)  I'm not very clear on how we would 
handle that with the current architect. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/include/clang/AST/RecordLayout.h:74
+  /// The maximum allowed field alignment. This is set by #pragma pack.
+  CharUnits MaxFieldAlignment;
+

efriedma wrote:
> If we have to keep around extra data anyway, probably better to add a field 
> for the preferred alignment, so we can keep the preferred alignment handling 
> together.
We could get the MaxFieldAlignment in the same way 
ItaniumRecordLayoutBuilder::InitializeLayout get. Do we need to keep an extra 
data?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

This approach seems to reflect the consensus from the mailing list.




Comment at: clang/include/clang/AST/RecordLayout.h:74
+  /// The maximum allowed field alignment. This is set by #pragma pack.
+  CharUnits MaxFieldAlignment;
+

If we have to keep around extra data anyway, probably better to add a field for 
the preferred alignment, so we can keep the preferred alignment handling 
together.



Comment at: clang/lib/AST/ASTContext.cpp:2518
+  (T->isSpecificBuiltinType(BuiltinType::LongDouble) &&
+   getTargetInfo().getTriple().isOSAIX()))
 // Don't increase the alignment if an alignment attribute was specified on 
a

We try to avoid OS-specific checks here.  But I'm not sure what this should 
look like on other targets.



Comment at: clang/lib/AST/ASTContext.cpp:2527
+// as its first member.
+if (getTargetInfo().getTriple().isOSAIX()) {
+  const RecordDecl *RD = RT->getDecl();

I'd prefer to centralize the check for "AIX struct layout" in one place, as 
opposed to putting checks for AIX targets in multiple places.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:3438
+C.getPreferredTypeAlign(RD->getTypeForDecl()))
+   .getQuantity());
 

Please don't make the dumping code lie about the "align".  If you want to dump 
the preferred alignment in addition, that would be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L created this revision.
Xiangling_L added reviewers: hubert.reinterpretcast, jasonliu, sfertile, 
cebowleratibm.
Xiangling_L added a project: LLVM.
Herald added subscribers: cfe-commits, kbarton, nemanjai.
Herald added a project: clang.

Implement AIX special alignment rule by recursively checking if the
'real' first member is a double/long double. If yes, then this member
should be naturally aligned to 8 rather than 4.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79719

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/RecordLayout.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/RecordLayout.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/PPC.h
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-no-unique-address-with-double.cpp
  clang/test/Layout/aix-virtual-function-and-base-with-double.cpp

Index: clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
===
--- /dev/null
+++ clang/test/Layout/aix-virtual-function-and-base-with-double.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | FileCheck \
+// RUN: --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc64-ibm-aix-xcoff \
+// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | FileCheck \
+// RUN: --check-prefixes=CHECK,CHECK64 %s
+
+namespace test1 {
+struct A {
+  double d1;
+  virtual void boo() {}
+};
+
+struct B {
+  double d2;
+  A a;
+};
+
+struct C : public A {
+  double d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::A
+// CHECK-NEXT:0 |   (A vtable pointer)
+// CHECK32-NEXT:  4 |   double d1
+// CHECK32-NEXT:| [sizeof=12, dsize=12, align=4,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4]
+// CHECK64-NEXT:  8 |   double d1
+// CHECK64-NEXT:| [sizeof=16, dsize=16, align=8,
+// CHECK64-NEXT:|  nvsize=16, nvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::B
+// CHECK-NEXT:0 |   double d2
+// CHECK-NEXT:8 |   struct test1::A a
+// CHECK-NEXT:8 | (A vtable pointer)
+// CHECK32-NEXT: 12 | double d1
+// CHECK32-NEXT:| [sizeof=24, dsize=20, align=8,
+// CHECK32-NEXT:|  nvsize=20, nvalign=8]
+// CHECK64-NEXT: 16 | double d1
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test1::C
+// CHECK-NEXT:0 |   struct test1::A (primary base)
+// CHECK-NEXT:0 | (A vtable pointer)
+// CHECK32-NEXT:  4 | double d1
+// CHECK32-NEXT: 12 |   double d3
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=4,
+// CHECK32-NEXT:|  nvsize=20, nvalign=4]
+// CHECK64-NEXT:  8 | double d1
+// CHECK64-NEXT: 16 |   double d3
+// CHECK64-NEXT:| [sizeof=24, dsize=24, align=8,
+// CHECK64-NEXT:|  nvsize=24, nvalign=8]
+
+}; // namespace test1
+
+namespace test2 {
+struct A {
+  long long l1;
+};
+
+struct B : public virtual A {
+  double d2;
+};
+
+#pragma pack(2)
+struct C : public virtual A {
+  double __attribute__((aligned(4))) d3;
+};
+
+int i = sizeof(B);
+int j = sizeof(C);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::A
+// CHECK-NEXT:0 |   long long l1
+// CHECK-NEXT:  | [sizeof=8, dsize=8, align=8,
+// CHECK-NEXT:  |  nvsize=8, nvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::B
+// CHECK-NEXT:0 |   (B vtable pointer)
+// CHECK32-NEXT:  4 |   double d2
+// CHECK64-NEXT:  8 |   double d2
+// CHECK-NEXT:   16 |   struct test2::A (virtual base)
+// CHECK-NEXT:   16 | long long l1
+// CHECK-NEXT:  | [sizeof=24, dsize=24, align=8,
+// CHECK32-NEXT:|  nvsize=12, nvalign=4]
+// CHECK64-NEXT:|  nvsize=16, nvalign=8]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:0 | struct test2::C
+// CHECK-NEXT:0 |   (C vtable pointer)
+// CHECK32-NEXT:  4 |   double d3
+// CHECK32-NEXT: 12 |   struct test2::A (virtual base)
+// CHECK32-NEXT: 12 | long long l1
+// CHECK32-NEXT:| [sizeof=20, dsize=20, align=2,
+// CHECK32-NEXT:|  nvsize=12, nvalign=2]
+// CHECK64-NEXT:  8 |   double d3
+// CHECK64-NEXT: 16 |   struct test2::A (virtual base)
+// CHECK64-NEXT: 16 | long long l1
+// CHECK64-NEXT: