[PATCH] D107941: Fix assertion when passing function into inline asm's input operand

[Jason Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfe177a1773e4: Fix assertion when passing function into 
inline asms input operand (authored by jasonliu).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107941

Files:
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/CodeGen/asm-call-func.c


Index: clang/test/CodeGen/asm-call-func.c
===
--- /dev/null
+++ clang/test/CodeGen/asm-call-func.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-unknown-linux-gnu| 
FileCheck %s
+
+void callee(void);
+void caller() {
+  //CHECK: call void asm sideeffect "rcall $0", 
"n,~{dirflag},~{fpsr},~{flags}"(void ()* @callee)
+  asm("rcall %0" ::"n"(callee));
+}
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -393,30 +393,31 @@
   diag::err_asm_invalid_lvalue_in_input)
  << Info.getConstraintStr()
  << InputExpr->getSourceRange());
-} else if (Info.requiresImmediateConstant() && !Info.allowsRegister()) {
-  if (!InputExpr->isValueDependent()) {
-Expr::EvalResult EVResult;
-if (InputExpr->EvaluateAsRValue(EVResult, Context, true)) {
-  // For compatibility with GCC, we also allow pointers that would be
-  // integral constant expressions if they were cast to int.
-  llvm::APSInt IntResult;
-  if (EVResult.Val.toIntegralConstant(IntResult, InputExpr->getType(),
-   Context))
-if (!Info.isValidAsmImmediate(IntResult))
-  return StmtError(Diag(InputExpr->getBeginLoc(),
-diag::err_invalid_asm_value_for_constraint)
-   << toString(IntResult, 10)
-   << Info.getConstraintStr()
-   << InputExpr->getSourceRange());
-}
-  }
-
 } else {
   ExprResult Result = DefaultFunctionArrayLvalueConversion(Exprs[i]);
   if (Result.isInvalid())
 return StmtError();
 
-  Exprs[i] = Result.get();
+  InputExpr = Exprs[i] = Result.get();
+
+  if (Info.requiresImmediateConstant() && !Info.allowsRegister()) {
+if (!InputExpr->isValueDependent()) {
+  Expr::EvalResult EVResult;
+  if (InputExpr->EvaluateAsRValue(EVResult, Context, true)) {
+// For compatibility with GCC, we also allow pointers that would be
+// integral constant expressions if they were cast to int.
+llvm::APSInt IntResult;
+if (EVResult.Val.toIntegralConstant(IntResult, 
InputExpr->getType(),
+Context))
+  if (!Info.isValidAsmImmediate(IntResult))
+return StmtError(
+Diag(InputExpr->getBeginLoc(),
+ diag::err_invalid_asm_value_for_constraint)
+<< toString(IntResult, 10) << Info.getConstraintStr()
+<< InputExpr->getSourceRange());
+  }
+}
+  }
 }
 
 if (Info.allowsRegister()) {


Index: clang/test/CodeGen/asm-call-func.c
===
--- /dev/null
+++ clang/test/CodeGen/asm-call-func.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-unknown-linux-gnu| FileCheck %s
+
+void callee(void);
+void caller() {
+  //CHECK: call void asm sideeffect "rcall $0", "n,~{dirflag},~{fpsr},~{flags}"(void ()* @callee)
+  asm("rcall %0" ::"n"(callee));
+}
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -393,30 +393,31 @@
   diag::err_asm_invalid_lvalue_in_input)
  << Info.getConstraintStr()
  << InputExpr->getSourceRange());
-} else if (Info.requiresImmediateConstant() && !Info.allowsRegister()) {
-  if (!InputExpr->isValueDependent()) {
-Expr::EvalResult EVResult;
-if (InputExpr->EvaluateAsRValue(EVResult, Context, true)) {
-  // For compatibility with GCC, we also allow pointers that would be
-  // integral constant expressions if they were cast to int.
-  llvm::APSInt IntResult;
-  if (EVResult.Val.toIntegralConstant(IntResult, InputExpr->getType(),
-   Context))
-if (!Info.isValidAsmImmediate(IntResult))
-  return StmtError(Diag(InputExpr->getBeginLoc(),
- 

[PATCH] D104314: [AIX] Remove --as-needed passing into aix linker

[Jason Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4e2aee8d3bab: [AIX] Remove --as-needed passing into aix 
linker (authored by jasonliu).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104314

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/aix-ld.c

Index: clang/test/Driver/aix-ld.c
===
--- clang/test/Driver/aix-ld.c
+++ clang/test/Driver/aix-ld.c
@@ -20,7 +20,9 @@
 // CHECK-LD32-NOT: "-lc++"
 // CHECK-LD32-NOT: "-lc++abi"
 // CHECK-LD32: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc.a"
+// CHECK-LD32-NOT: "--as-needed"
 // CHECK-LD32: "-lunwind"
+// CHECK-LD32-NOT: "--no-as-needed"
 // CHECK-LD32-NOT: "-lm"
 // CHECK-LD32: "-lc"
 
@@ -43,7 +45,9 @@
 // CHECK-LD64-NOT: "-lc++"
 // CHECK-LD64-NOT: "-lc++abi"
 // CHECK-LD64: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc64.a"
+// CHECK-LD64-NOT: "--as-needed"
 // CHECK-LD64: "-lunwind"
+// CHECK-LD64-NOT: "--no-as-needed"
 // CHECK-LD64-NOT: "-lm"
 // CHECK-LD64: "-lc"
 
@@ -67,7 +71,9 @@
 // CHECK-LD32-PTHREAD-NOT: "-lc++"
 // CHECK-LD32-PTHREAD-NOT: "-lc++abi"
 // CHECK-LD32-PTHREAD: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc.a"
+// CHECK-LD32-PTHREAD-NOT: "--as-needed"
 // CHECK-LD32-PTHREAD: "-lunwind"
+// CHECK-LD32-PTHREAD-NOT: "--no-as-needed"
 // CHECK-LD32-PTHREAD: "-lpthreads"
 // CHECK-LD32-PTHREAD-NOT: "-lm"
 // CHECK-LD32-PTHREAD: "-lc"
@@ -92,7 +98,9 @@
 // CHECK-LD64-PTHREAD-NOT: "-lc++"
 // CHECK-LD64-PTHREAD-NOT: "-lc++abi"
 // CHECK-LD64-PTHREAD: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc64.a"
+// CHECK-LD64-PTHREAD-NOT: "--as-needed"
 // CHECK-LD64-PTHREAD: "-lunwind"
+// CHECK-LD64-PTHREAD-NOT: "--no-as-needed"
 // CHECK-LD64-PTHREAD: "-lpthreads"
 // CHECK-LD64-PTHREAD-NOT: "-lm"
 // CHECK-LD64-PTHREAD: "-lc"
@@ -117,7 +125,9 @@
 // CHECK-LD32-PROF-NOT: "-lc++"
 // CHECK-LD32-PROF-NOT: "-lc++abi"
 // CHECK-LD32-PROF: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc.a"
+// CHECK-LD32-PROF-NOT: "--as-needed"
 // CHECK-LD32-PROF: "-lunwind"
+// CHECK-LD32-PROF-NOT: "--no-as-needed"
 // CHECK-LD32-PROF-NOT: "-lm"
 // CHECK-LD32-PROF: "-lc"
 
@@ -141,7 +151,9 @@
 // CHECK-LD64-GPROF-NOT: "-lc++"
 // CHECK-LD64-GPROF-NOT: "-lc++abi"
 // CHECK-LD64-GPROF: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc64.a"
+// CHECK-LD64-GPROF-NOT: "--as-needed"
 // CHECK-LD64-GPROF: "-lunwind"
+// CHECK-LD64-GPROF-NOT: "--no-as-needed"
 // CHECK-LD64-GPROF-NOT: "-lm"
 // CHECK-LD64-GPROF: "-lc"
 
@@ -165,7 +177,9 @@
 // CHECK-LD32-STATIC-NOT: "-lc++"
 // CHECK-LD32-STATIC-NOT: "-lc++abi"
 // CHECK-LD32-STATIC: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc.a"
+// CHECK-LD32-STATIC-NOT: "--as-needed"
 // CHECK-LD32-STATIC-NOT: "-lunwind"
+// CHECK-LD32-STATIC-NOT: "--no-as-needed"
 // CHECK-LD32-STATIC-NOT: "-lm"
 // CHECK-LD32-STATIC: "-lc"
 
@@ -190,7 +204,9 @@
 // CHECK-LD32-LIBP-NOT: "-lc++"
 // CHECK-LD32-LIBP-NOT: "-lc++abi"
 // CHECK-LD32-LIBP: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc.a"
+// CHECK-LD32-LIBP-NOT: "--as-needed"
 // CHECK-LD32-LIBP: "-lunwind"
+// CHECK-LD32-LIBP-NOT: "--no-as-needed"
 // CHECK-LD32-LIBP-NOT: "-lm"
 // CHECK-LD32-LIBP: "-lc"
 
@@ -215,7 +231,9 @@
 // CHECK-LD32-NO-STD-LIB-NOT: "-lc++"
 // CHECK-LD32-NO-STD-LIB-NOT: "-lc++abi"
 // CHECK-LD32-NO-STD-LIB-NOT: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc.a"
+// CHECK-LD32-NO-STD-LIB-NOT: "--as-needed"
 // CHECK-LD32-NO-STD-LIB-NOT: "-lunwind"
+// CHECK-LD32-NO-STD-LIB-NOT: "--no-as-needed"
 // CHECK-LD32-NO-STD-LIB-NOT: "-lpthreads"
 // CHECK-LD32-NO-STD-LIB-NOT: "-lm"
 // CHECK-LD32-NO-STD-LIB-NOT: "-lc"
@@ -241,7 +259,9 @@
 // CHECK-LD64-NO-DEFAULT-LIBS-NOT: "-lc++"
 // CHECK-LD64-NO-DEFAULT-LIBS-NOT: "-lc++abi"
 // CHECK-LD64-NO-DEFAULT-LIBS-NOT: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc64.a"
+// CHECK-LD64-NO-DEFAULT-LIBS-NOT: "--as-needed"
 // CHECK-LD64-NO-DEFAULT-LIBS-NOT: "-lunwind"
+// CHECK-LD64-NO-DEFAULT-LIBS-NOT: "--no-as-needed"
 // CHECK-LD64-NO-DEFAULT-LIBS-NOT: "-lpthreads"
 // CHECK-LD64-NO-DEFAULT-LIBS-NOT: "-lm"
 // CHECK-LD64-NO-DEFAULT-LIBS-NOT: "-lc"
@@ -268,7 +288,9 @@
 // CHECK-LD32-ARG-ORDER-NOT: "-lc++"
 // CHECK-LD32-ARG-ORDER-NOT: "-lc++abi"
 // CHECK-LD32-ARG-ORDER: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc.a"
+// CHECK-LD32-ARG-ORDER-NOT: "--as-needed"
 // 

[PATCH] D102813: [AIX] Add -lc++abi and -lunwind for linking

[Jason Liu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7922ff601094: [AIX] Add -lc++abi and -lunwind for linking 
(authored by jasonliu).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102813

Files:
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/AIX.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/aix-ld.c

Index: clang/test/Driver/aix-ld.c
===
--- clang/test/Driver/aix-ld.c
+++ clang/test/Driver/aix-ld.c
@@ -18,7 +18,9 @@
 // CHECK-LD32: "[[SYSROOT]]/usr/lib{{/|}}crt0.o"
 // CHECK-LD32: "[[SYSROOT]]/usr/lib{{/|}}crti.o"
 // CHECK-LD32-NOT: "-lc++"
+// CHECK-LD32-NOT: "-lc++abi"
 // CHECK-LD32: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc.a"
+// CHECK-LD32: "-lunwind"
 // CHECK-LD32-NOT: "-lm"
 // CHECK-LD32: "-lc"
 
@@ -39,7 +41,9 @@
 // CHECK-LD64: "[[SYSROOT]]/usr/lib{{/|}}crt0_64.o"
 // CHECK-LD64: "[[SYSROOT]]/usr/lib{{/|}}crti_64.o"
 // CHECK-LD64-NOT: "-lc++"
+// CHECK-LD64-NOT: "-lc++abi"
 // CHECK-LD64: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc64.a"
+// CHECK-LD64: "-lunwind"
 // CHECK-LD64-NOT: "-lm"
 // CHECK-LD64: "-lc"
 
@@ -61,7 +65,9 @@
 // CHECK-LD32-PTHREAD: "[[SYSROOT]]/usr/lib{{/|}}crt0.o"
 // CHECK-LD32-PTHREAD: "[[SYSROOT]]/usr/lib{{/|}}crti.o"
 // CHECK-LD32-PTHREAD-NOT: "-lc++"
+// CHECK-LD32-PTHREAD-NOT: "-lc++abi"
 // CHECK-LD32-PTHREAD: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc.a"
+// CHECK-LD32-PTHREAD: "-lunwind"
 // CHECK-LD32-PTHREAD: "-lpthreads"
 // CHECK-LD32-PTHREAD-NOT: "-lm"
 // CHECK-LD32-PTHREAD: "-lc"
@@ -84,7 +90,9 @@
 // CHECK-LD64-PTHREAD: "[[SYSROOT]]/usr/lib{{/|}}crt0_64.o"
 // CHECK-LD64-PTHREAD: "[[SYSROOT]]/usr/lib{{/|}}crti_64.o"
 // CHECK-LD64-PTHREAD-NOT: "-lc++"
+// CHECK-LD64-PTHREAD-NOT: "-lc++abi"
 // CHECK-LD64-PTHREAD: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc64.a"
+// CHECK-LD64-PTHREAD: "-lunwind"
 // CHECK-LD64-PTHREAD: "-lpthreads"
 // CHECK-LD64-PTHREAD-NOT: "-lm"
 // CHECK-LD64-PTHREAD: "-lc"
@@ -107,7 +115,9 @@
 // CHECK-LD32-PROF: "[[SYSROOT]]/usr/lib{{/|}}mcrt0.o"
 // CHECK-LD32-PROF: "[[SYSROOT]]/usr/lib{{/|}}crti.o"
 // CHECK-LD32-PROF-NOT: "-lc++"
+// CHECK-LD32-PROF-NOT: "-lc++abi"
 // CHECK-LD32-PROF: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc.a"
+// CHECK-LD32-PROF: "-lunwind"
 // CHECK-LD32-PROF-NOT: "-lm"
 // CHECK-LD32-PROF: "-lc"
 
@@ -129,7 +139,9 @@
 // CHECK-LD64-GPROF: "[[SYSROOT]]/usr/lib{{/|}}gcrt0_64.o"
 // CHECK-LD64-GPROF: "[[SYSROOT]]/usr/lib{{/|}}crti_64.o"
 // CHECK-LD64-GPROF-NOT: "-lc++"
+// CHECK-LD64-GPROF-NOT: "-lc++abi"
 // CHECK-LD64-GPROF: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc64.a"
+// CHECK-LD64-GPROF: "-lunwind"
 // CHECK-LD64-GPROF-NOT: "-lm"
 // CHECK-LD64-GPROF: "-lc"
 
@@ -151,7 +163,9 @@
 // CHECK-LD32-STATIC: "[[SYSROOT]]/usr/lib{{/|}}crt0.o"
 // CHECK-LD32-STATIC: "[[SYSROOT]]/usr/lib{{/|}}crti.o"
 // CHECK-LD32-STATIC-NOT: "-lc++"
+// CHECK-LD32-STATIC-NOT: "-lc++abi"
 // CHECK-LD32-STATIC: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc.a"
+// CHECK-LD32-STATIC-NOT: "-lunwind"
 // CHECK-LD32-STATIC-NOT: "-lm"
 // CHECK-LD32-STATIC: "-lc"
 
@@ -174,7 +188,9 @@
 // CHECK-LD32-LIBP: "[[SYSROOT]]/usr/lib{{/|}}crti.o"
 // CHECK-LD32-LIBP: "-L[[SYSROOT]]/powerpc-ibm-aix7.1.0.0"
 // CHECK-LD32-LIBP-NOT: "-lc++"
+// CHECK-LD32-LIBP-NOT: "-lc++abi"
 // CHECK-LD32-LIBP: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc.a"
+// CHECK-LD32-LIBP: "-lunwind"
 // CHECK-LD32-LIBP-NOT: "-lm"
 // CHECK-LD32-LIBP: "-lc"
 
@@ -197,7 +213,9 @@
 // CHECK-LD32-NO-STD-LIB-NOT: "[[SYSROOT]]/usr/lib{{/|}}crt0.o"
 // CHECK-LD32-NO-STD-LIB-NOT: "[[SYSROOT]]/usr/lib{{/|}}crti.o"
 // CHECK-LD32-NO-STD-LIB-NOT: "-lc++"
+// CHECK-LD32-NO-STD-LIB-NOT: "-lc++abi"
 // CHECK-LD32-NO-STD-LIB-NOT: "[[RESOURCE_DIR]]{{/|}}lib{{/|}}aix{{/|}}libclang_rt.builtins-powerpc.a"
+// CHECK-LD32-NO-STD-LIB-NOT: "-lunwind"
 // CHECK-LD32-NO-STD-LIB-NOT: "-lpthreads"
 // CHECK-LD32-NO-STD-LIB-NOT: "-lm"
 // CHECK-LD32-NO-STD-LIB-NOT: "-lc"
@@ -221,7 +239,9 @@
 // CHECK-LD64-NO-DEFAULT-LIBS: "[[SYSROOT]]/usr/lib{{/|}}crt0_64.o"
 // CHECK-LD64-NO-DEFAULT-LIBS: "[[SYSROOT]]/usr/lib{{/|}}crti_64.o"
 // CHECK-LD64-NO-DEFAULT-LIBS-NOT: "-lc++"
+// 

[PATCH] D102724: Revert "[AIX] Avoid structor alias; die before bad alias codegen"

[Jason Liu via Phabricator via cfe-commits]

jasonliu added a comment.

Good idea, fyi, this is the patch: https://reviews.llvm.org/D83252


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102724

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

[PATCH] D102724: Revert "[AIX] Avoid structor alias; die before bad alias codegen"

[Jason Liu via Phabricator via cfe-commits]

jasonliu accepted this revision.
jasonliu 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/D102724/new/

https://reviews.llvm.org/D102724

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

[PATCH] D102724: Revert "[AIX] Avoid structor alias; die before bad alias codegen"

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4964
   if (!RawTriple.isOSDarwin() && !RawTriple.isNVPTX() &&
-  !RawTriple.isAMDGPU() && !RawTriple.isOSAIX())
+  !RawTriple.isAMDGPU())
 CmdArgs.push_back("-mconstructor-aliases");

Format as clang-format suggest. You could run clang-format or git-clang-format 
just for your commits by the way. 



Comment at: clang/test/Driver/aix-constructor-alias.c:3
-
-// RUN: %clang -### -target powerpc-ibm-aix7.1.0.0 %s -c -o %t.o 2>&1 \
-// RUN:   | FileCheck %s

Instead of removing this file, we want to check if `-mconstructor-aliases` is 
passed in for AIX.
Check if there are other existing files doing it for other target, then you 
just need to add the check for AIX target in other files.



Comment at: llvm/test/CodeGen/PowerPC/aix-alias.ll:4
-
-; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
-; RUN: -mattr=-altivec -data-sections=false -xcoff-traceback-table=false < 
%s | \

I don't think we want to remove the testing of our alias implementation. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102724

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

[PATCH] D99291: [AIX] Support init priority attribute

[Jason Liu via Phabricator via cfe-commits]

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

LGTM.




Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:663
 
+  // Create our global prioritized cleanup function.
+  if (!PrioritizedCXXStermFinalizers.empty()) {

Just noting that this trunk of code have very similar logic counter part in 
`EmitCXXGlobalInitFunc`. It seems there is no easy way to common them up given 
the current way of implementing it. It may require a non-trivial amount of 
refactoring for us to common up the similar logic between init and cleanup. 


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

https://reviews.llvm.org/D99291

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Jason Liu via Phabricator via cfe-commits]

jasonliu accepted this revision.
jasonliu added a comment.

LGTM with minor nits.




Comment at: clang/lib/Sema/SemaAttr.cpp:74
 return;
-  // The #pragma align/pack affected a record in an included file, so Clang
-  // should warn when that the pragma was written in a file that included the
+  // The #pragma align/pack affected a record in an included file,  so Clang
+  // should warn when that pragma was written in a file that included the





Comment at: clang/lib/Sema/SemaAttr.cpp:347-349
+AlignmentVal = CurVal.getPackNumber();
+if (!CurVal.IsPackSet())
   AlignmentVal = 8;

nit



Comment at: clang/lib/Sema/SemaAttr.cpp:372
+  // XL pragma pack does not support identifier syntax.
+  if (IsXLPragma && !SlotLabel.empty()) {
+Diag(PragmaLoc, diag::err_pragma_pack_identifer_not_supported);

Could we move this to the top of the function so that we could have a quick 
exit when we sees this?
If we are going to emit an error, the `AlignPackStack.Act(PragmaLoc, Action, 
StringRef(), Info);` is not necessary any more.



Comment at: clang/lib/Sema/SemaAttr.cpp:555
+  });
+  // If we found the label so pop from there.
+  if (I != Stack.rend()) {





Comment at: clang/lib/Sema/SemaAttr.cpp:581
+} else if (!Stack.empty()) {
+  // xl '#pragma align' sets the base line,
+  // and pragma pack cannot pop over the base line.





Comment at: clang/lib/Sema/SemaAttr.cpp:582
+  // xl '#pragma align' sets the base line,
+  // and pragma pack cannot pop over the base line.
+  if (Value.IsXLStack() && Value.IsPackAttr() && 
CurrentValue.IsAlignAttr())




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

https://reviews.llvm.org/D87702

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:505
+// AlignPackInfo::getFromRawEncoding, it should not be inspected directly.
+uint32_t getRawEncoding(const AlignPackInfo ) const {
+  std::uint32_t Encoding{};

this getter do not need to take any parameters. You could just use `*this`.
Or, to make it consistent with the getFromRawEncoding, we could make this a 
static function as well and keep the parameter.



Comment at: clang/include/clang/Sema/Sema.h:506
+uint32_t getRawEncoding(const AlignPackInfo ) const {
+  std::uint32_t Encoding{};
+  if (Info.IsAIXStack())

Since this new structure have the same size as the uint32_t, have you consider 
using memcpy to burn the data directly into the uint32_t, and vice versa?



Comment at: clang/include/clang/Sema/Sema.h:570
+/// \brief True if it is a AIX #pragma align/pack stack.
+bool AIXStack;
+

It might be better to rename it XLStack, as this is the XL compiler behavior on 
AIX. Other compilers on AIX, like gcc, could have different behavior.



Comment at: clang/include/clang/Sema/Sema.h:637
   PragmaStack VtorDispStack;
-  // #pragma pack.
-  // Sentinel to represent when the stack is set to mac68k alignment.
-  static const unsigned kMac68kAlignmentSentinel = ~0U;
-  PragmaStack PackStack;
+  PragmaStack PackStack;
   // The current #pragma pack values and locations at each #include.

Xiangling_L wrote:
> jasonliu wrote:
> > We should consider renaming PackStack to AlignPackStack across Clang. Maybe 
> > even as a NFC first. As it is right now, clang already uses one stack to 
> > record those two informations from `Pragma align` and `Pragma pack`. Leave 
> > it as PackStack is too confusing, and people could actually ignore the 
> > pragma Align when developing with PackStack. 
> That's a good point. I will create a NFC accordingly.
Please post this NFC when you have time.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:699-700
+IsMac68kAlign(false),
+IsNaturalAlign(!Context.getTargetInfo().getTriple().isOSAIX() ? true
+  : false),
+IsMsStruct(false), UnfilledBitsInLastUnit(0),





Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1322
   HandledFirstNonOverlappingEmptyField =
-  !Context.getTargetInfo().defaultsToAIXPowerAlignment();
+  !Context.getTargetInfo().defaultsToAIXPowerAlignment() || IsNaturalAlign;
 

Not sure if this would have a real effect, but reading this code concerns me a 
little bit:
We would set `HandledFirstNonOverlappingEmptyField` based on `IsNaturalAlign`, 
but for AIX, that bit might only get set at line 1343.
It would look like we have a read before write to the variable here. 



Comment at: clang/lib/Sema/SemaAttr.cpp:232
 Action = Sema::PSK_Push_Set;
-Alignment = 0;
+if (IsAIX)
+  ModeVal = AlignPackInfo::Natural;

XLPragmaPack is used to control pack and align stack semantic behavior.
Someone on AIX might want to turn off this compatible pack and align with XL 
behavior (to get the compatible behavior with clang on other platform for 
easier porting purpose), but they would still prefer to have the natural 
alignment to work correctly.
So it might not be desirable to query this option for this natural alignment 
behavior.



Comment at: clang/lib/Sema/SemaAttr.cpp:413
+  // FIXME: PackStack may contain both #pragma align and #pragma pack
+  // information, we should warn about both modified align mode and alignment.
   if (PrevPackState.CurrentValue != PackStack.CurrentValue) {

I don't think there is a preferable solution yet. Pointing out what could go 
wrong is good enough here.
Same for the FIXME below.


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

https://reviews.llvm.org/D87702

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

[PATCH] D91455: [XCOFF][AIX] Generate LSDA data and compact unwind section on AIX

[Jason Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa65d8c5d720d: [XCOFF][AIX] Generate LSDA data and compact 
unwind section on AIX (authored by jasonliu).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D91455?vs=308752=309007#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91455

Files:
  clang/lib/CodeGen/CGCleanup.h
  clang/lib/CodeGen/CGException.cpp
  clang/test/CodeGenCXX/personality.cpp
  llvm/include/llvm/Analysis/EHPersonalities.h
  llvm/include/llvm/CodeGen/AsmPrinter.h
  llvm/include/llvm/MC/MCTargetOptions.h
  llvm/lib/Analysis/EHPersonalities.cpp
  llvm/lib/CodeGen/AsmPrinter/AIXException.cpp
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
  llvm/lib/CodeGen/AsmPrinter/CMakeLists.txt
  llvm/lib/CodeGen/AsmPrinter/DwarfException.h
  llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/MC/MCAsmInfoXCOFF.cpp
  llvm/lib/MC/MCObjectFileInfo.cpp
  llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/test/CodeGen/PowerPC/aix-exception.ll

Index: llvm/test/CodeGen/PowerPC/aix-exception.ll
===
--- /dev/null
+++ llvm/test/CodeGen/PowerPC/aix-exception.ll
@@ -0,0 +1,152 @@
+; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec < %s | \
+; RUN:   FileCheck --check-prefixes=ASM,ASM32 %s
+
+; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec < %s | \
+; RUN:   FileCheck --check-prefixes=ASM,ASM64 %s
+
+@_ZTIi = external constant i8*
+
+define void @_Z9throwFuncv() {
+entry:
+  %exception = call i8* @__cxa_allocate_exception(i32 4) #2
+  %0 = bitcast i8* %exception to i32*
+  store i32 1, i32* %0, align 16
+  call void @__cxa_throw(i8* %exception, i8* bitcast (i8** @_ZTIi to i8*), i8* null) #3
+  unreachable
+}
+
+; ASM:._Z9throwFuncv:
+; ASM:  bl .__cxa_allocate_exception[PR]
+; ASM:  nop
+; ASM32:lwz 4, L..C0(2)
+; ASM64:ld 4, L..C0(2)
+; ASM:  bl .__cxa_throw[PR]
+; ASM:  nop
+
+define i32 @_Z9catchFuncv() personality i8* bitcast (i32 (...)* @__xlcxx_personality_v1 to i8*) {
+entry:
+  %retval = alloca i32, align 4
+  %exn.slot = alloca i8*, align 4
+  %ehselector.slot = alloca i32, align 4
+  %0 = alloca i32, align 4
+  invoke void @_Z9throwFuncv()
+  to label %invoke.cont unwind label %lpad
+
+invoke.cont:  ; preds = %entry
+  br label %try.cont
+
+lpad: ; preds = %entry
+  %1 = landingpad { i8*, i32 }
+  catch i8* bitcast (i8** @_ZTIi to i8*)
+  %2 = extractvalue { i8*, i32 } %1, 0
+  store i8* %2, i8** %exn.slot, align 4
+  %3 = extractvalue { i8*, i32 } %1, 1
+  store i32 %3, i32* %ehselector.slot, align 4
+  br label %catch.dispatch
+
+catch.dispatch:   ; preds = %lpad
+  %sel = load i32, i32* %ehselector.slot, align 4
+  %4 = call i32 @llvm.eh.typeid.for(i8* bitcast (i8** @_ZTIi to i8*)) #2
+  %matches = icmp eq i32 %sel, %4
+  br i1 %matches, label %catch, label %eh.resume
+
+catch:; preds = %catch.dispatch
+  %exn = load i8*, i8** %exn.slot, align 4
+  %5 = call i8* @__cxa_begin_catch(i8* %exn) #2
+  %6 = bitcast i8* %5 to i32*
+  %7 = load i32, i32* %6, align 4
+  store i32 %7, i32* %0, align 4
+  store i32 2, i32* %retval, align 4
+  call void @__cxa_end_catch() #2
+  br label %return
+
+try.cont: ; preds = %invoke.cont
+  store i32 1, i32* %retval, align 4
+  br label %return
+
+return:   ; preds = %try.cont, %catch
+  %8 = load i32, i32* %retval, align 4
+  ret i32 %8
+
+eh.resume:; preds = %catch.dispatch
+  %exn1 = load i8*, i8** %exn.slot, align 4
+  %sel2 = load i32, i32* %ehselector.slot, align 4
+  %lpad.val = insertvalue { i8*, i32 } undef, i8* %exn1, 0
+  %lpad.val3 = insertvalue { i8*, i32 } %lpad.val, i32 %sel2, 1
+  resume { i8*, i32 } %lpad.val3
+}
+
+; ASM:  ._Z9catchFuncv:
+; ASM:  L..func_begin0:
+; ASM:  # %bb.0:# %entry
+; ASM:  	mflr 0
+; ASM:  L..tmp0:
+; ASM:  	bl ._Z9throwFuncv
+; ASM:  	nop
+; ASM:  L..tmp1:
+; ASM:  # %bb.1:# %invoke.cont
+; ASM:  	li 3, 1
+; ASM:  L..BB1_2:   # %return
+; ASM:  	mtlr 0
+; ASM:  	blr
+; ASM:  L..BB1_3:   # %lpad
+; ASM:  L..tmp2:
+; ASM:  	bl .__cxa_begin_catch[PR]
+; ASM:  	nop
+; ASM:  	bl .__cxa_end_catch[PR]
+; ASM:  	nop
+; ASM:  	b L..BB1_2
+; ASM:  L..func_end0:
+
+; ASM:  	

[PATCH] D91455: [XCOFF][AIX] Generate LSDA data and compact unwind section on AIX

[Jason Liu via Phabricator via cfe-commits]

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



Comment at: llvm/test/CodeGen/PowerPC/aix-exception.ll:108-109
+; ASM: .byte   255 # @LPStart Encoding = 
omit
+; ASM32:   .byte   187 # @TType Encoding = 

+; ASM64:  .byte188 # @TType Encoding = 

+; ASM: .uleb128 L..ttbase0-L..ttbaseref0

hubert.reinterpretcast wrote:
> Is there a plan to update the annotation output here?
updated to address comment. 


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

https://reviews.llvm.org/D91455

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

[PATCH] D91455: [XCOFF][AIX] Generate LSDA data and compact unwind section on AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 308752.
jasonliu added a comment.

Address comments.


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

https://reviews.llvm.org/D91455

Files:
  clang/lib/CodeGen/CGCleanup.h
  clang/lib/CodeGen/CGException.cpp
  clang/test/CodeGenCXX/personality.cpp
  llvm/include/llvm/Analysis/EHPersonalities.h
  llvm/include/llvm/CodeGen/AsmPrinter.h
  llvm/include/llvm/MC/MCTargetOptions.h
  llvm/lib/Analysis/EHPersonalities.cpp
  llvm/lib/CodeGen/AsmPrinter/AIXException.cpp
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
  llvm/lib/CodeGen/AsmPrinter/CMakeLists.txt
  llvm/lib/CodeGen/AsmPrinter/DwarfException.h
  llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/MC/MCAsmInfoXCOFF.cpp
  llvm/lib/MC/MCObjectFileInfo.cpp
  llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/test/CodeGen/PowerPC/aix-exception.ll

Index: llvm/test/CodeGen/PowerPC/aix-exception.ll
===
--- /dev/null
+++ llvm/test/CodeGen/PowerPC/aix-exception.ll
@@ -0,0 +1,152 @@
+; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec < %s | \
+; RUN:   FileCheck --check-prefixes=ASM,ASM32 %s
+
+; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec < %s | \
+; RUN:   FileCheck --check-prefixes=ASM,ASM64 %s
+
+@_ZTIi = external constant i8*
+
+define void @_Z9throwFuncv() {
+entry:
+  %exception = call i8* @__cxa_allocate_exception(i32 4) #2
+  %0 = bitcast i8* %exception to i32*
+  store i32 1, i32* %0, align 16
+  call void @__cxa_throw(i8* %exception, i8* bitcast (i8** @_ZTIi to i8*), i8* null) #3
+  unreachable
+}
+
+; ASM:._Z9throwFuncv:
+; ASM:  bl .__cxa_allocate_exception[PR]
+; ASM:  nop
+; ASM32:lwz 4, L..C0(2)
+; ASM64:ld 4, L..C0(2)
+; ASM:  bl .__cxa_throw[PR]
+; ASM:  nop
+
+define i32 @_Z9catchFuncv() personality i8* bitcast (i32 (...)* @__xlcxx_personality_v1 to i8*) {
+entry:
+  %retval = alloca i32, align 4
+  %exn.slot = alloca i8*, align 4
+  %ehselector.slot = alloca i32, align 4
+  %0 = alloca i32, align 4
+  invoke void @_Z9throwFuncv()
+  to label %invoke.cont unwind label %lpad
+
+invoke.cont:  ; preds = %entry
+  br label %try.cont
+
+lpad: ; preds = %entry
+  %1 = landingpad { i8*, i32 }
+  catch i8* bitcast (i8** @_ZTIi to i8*)
+  %2 = extractvalue { i8*, i32 } %1, 0
+  store i8* %2, i8** %exn.slot, align 4
+  %3 = extractvalue { i8*, i32 } %1, 1
+  store i32 %3, i32* %ehselector.slot, align 4
+  br label %catch.dispatch
+
+catch.dispatch:   ; preds = %lpad
+  %sel = load i32, i32* %ehselector.slot, align 4
+  %4 = call i32 @llvm.eh.typeid.for(i8* bitcast (i8** @_ZTIi to i8*)) #2
+  %matches = icmp eq i32 %sel, %4
+  br i1 %matches, label %catch, label %eh.resume
+
+catch:; preds = %catch.dispatch
+  %exn = load i8*, i8** %exn.slot, align 4
+  %5 = call i8* @__cxa_begin_catch(i8* %exn) #2
+  %6 = bitcast i8* %5 to i32*
+  %7 = load i32, i32* %6, align 4
+  store i32 %7, i32* %0, align 4
+  store i32 2, i32* %retval, align 4
+  call void @__cxa_end_catch() #2
+  br label %return
+
+try.cont: ; preds = %invoke.cont
+  store i32 1, i32* %retval, align 4
+  br label %return
+
+return:   ; preds = %try.cont, %catch
+  %8 = load i32, i32* %retval, align 4
+  ret i32 %8
+
+eh.resume:; preds = %catch.dispatch
+  %exn1 = load i8*, i8** %exn.slot, align 4
+  %sel2 = load i32, i32* %ehselector.slot, align 4
+  %lpad.val = insertvalue { i8*, i32 } undef, i8* %exn1, 0
+  %lpad.val3 = insertvalue { i8*, i32 } %lpad.val, i32 %sel2, 1
+  resume { i8*, i32 } %lpad.val3
+}
+
+; ASM:  ._Z9catchFuncv:
+; ASM:  L..func_begin0:
+; ASM:  # %bb.0:# %entry
+; ASM:  	mflr 0
+; ASM:  L..tmp0:
+; ASM:  	bl ._Z9throwFuncv
+; ASM:  	nop
+; ASM:  L..tmp1:
+; ASM:  # %bb.1:# %invoke.cont
+; ASM:  	li 3, 1
+; ASM:  L..BB1_2:   # %return
+; ASM:  	mtlr 0
+; ASM:  	blr
+; ASM:  L..BB1_3:   # %lpad
+; ASM:  L..tmp2:
+; ASM:  	bl .__cxa_begin_catch[PR]
+; ASM:  	nop
+; ASM:  	bl .__cxa_end_catch[PR]
+; ASM:  	nop
+; ASM:  	b L..BB1_2
+; ASM:  L..func_end0:
+
+; ASM:  	.csect .gcc_except_table[RO],2
+; ASM:  	.align	2
+; ASM:  GCC_except_table1:
+; ASM:  L..exception0:
+; ASM:  	.byte	255 # @LPStart Encoding = omit
+; ASM32:	.byte	187 # @TType Encoding = indirect 

[PATCH] D91455: [XCOFF][AIX] Generate LSDA data and compact unwind section on AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: llvm/lib/CodeGen/AsmPrinter/AIXException.cpp:68
+Per = dyn_cast(F.getPersonalityFn()->stripPointerCasts());
+  bool EmitEHBlock =
+  HasLandingPads || (F.hasPersonalityFn() &&

daltenty wrote:
> This logic seems very similar to the base class. 
> 
> The pattern there and in other instance of EHStreamer seems to be to make 
> these queries in beginFunction, store the results in a member and just early 
> exit if we have nothing to emit in endFunction, etc. Is that something we 
> should be doing here? (e.g. presumably the traceback emission will want to 
> know if we plan to emit anything so it can emit the appropriate info)
I agree that this is a query that we want to share with the traceback table 
emission. 
Presumably, we need to do the traceback table emission somewhere in 
PPCAIXAsmPrinter.
So that means we would need to have a query that could accessible from 
PPCAIXAsmPrinter and here. 
And there are other EHStreamer that have similar queries (but not all of them), 
so this makes it trickier to get it right if we want to do it for all 
platforms. 
I was hoping to address this issue when we actually do the traceback table 
emission for EH info so that we could keep the scope of the patch reasonable.


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

https://reviews.llvm.org/D91455

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

[PATCH] D91455: [XCOFF][AIX] Generate LSDA data and compact unwind section on AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 308653.
jasonliu added a comment.

Address comment.


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

https://reviews.llvm.org/D91455

Files:
  clang/lib/CodeGen/CGCleanup.h
  clang/lib/CodeGen/CGException.cpp
  clang/test/CodeGenCXX/personality.cpp
  llvm/include/llvm/Analysis/EHPersonalities.h
  llvm/include/llvm/CodeGen/AsmPrinter.h
  llvm/include/llvm/MC/MCTargetOptions.h
  llvm/lib/Analysis/EHPersonalities.cpp
  llvm/lib/CodeGen/AsmPrinter/AIXException.cpp
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
  llvm/lib/CodeGen/AsmPrinter/CMakeLists.txt
  llvm/lib/CodeGen/AsmPrinter/DwarfException.h
  llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/MC/MCAsmInfoXCOFF.cpp
  llvm/lib/MC/MCObjectFileInfo.cpp
  llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/test/CodeGen/PowerPC/aix-exception.ll

Index: llvm/test/CodeGen/PowerPC/aix-exception.ll
===
--- /dev/null
+++ llvm/test/CodeGen/PowerPC/aix-exception.ll
@@ -0,0 +1,152 @@
+; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec < %s | \
+; RUN:   FileCheck --check-prefixes=ASM,ASM32 %s
+
+; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec < %s | \
+; RUN:   FileCheck --check-prefixes=ASM,ASM64 %s
+
+@_ZTIi = external constant i8*
+
+define void @_Z9throwFuncv() {
+entry:
+  %exception = call i8* @__cxa_allocate_exception(i32 4) #2
+  %0 = bitcast i8* %exception to i32*
+  store i32 1, i32* %0, align 16
+  call void @__cxa_throw(i8* %exception, i8* bitcast (i8** @_ZTIi to i8*), i8* null) #3
+  unreachable
+}
+
+; ASM:._Z9throwFuncv:
+; ASM:  bl .__cxa_allocate_exception[PR]
+; ASM:  nop
+; ASM32:lwz 4, L..C0(2)
+; ASM64:ld 4, L..C0(2)
+; ASM:  bl .__cxa_throw[PR]
+; ASM:  nop
+
+define i32 @_Z9catchFuncv() personality i8* bitcast (i32 (...)* @__xlcxx_personality_v1 to i8*) {
+entry:
+  %retval = alloca i32, align 4
+  %exn.slot = alloca i8*, align 4
+  %ehselector.slot = alloca i32, align 4
+  %0 = alloca i32, align 4
+  invoke void @_Z9throwFuncv()
+  to label %invoke.cont unwind label %lpad
+
+invoke.cont:  ; preds = %entry
+  br label %try.cont
+
+lpad: ; preds = %entry
+  %1 = landingpad { i8*, i32 }
+  catch i8* bitcast (i8** @_ZTIi to i8*)
+  %2 = extractvalue { i8*, i32 } %1, 0
+  store i8* %2, i8** %exn.slot, align 4
+  %3 = extractvalue { i8*, i32 } %1, 1
+  store i32 %3, i32* %ehselector.slot, align 4
+  br label %catch.dispatch
+
+catch.dispatch:   ; preds = %lpad
+  %sel = load i32, i32* %ehselector.slot, align 4
+  %4 = call i32 @llvm.eh.typeid.for(i8* bitcast (i8** @_ZTIi to i8*)) #2
+  %matches = icmp eq i32 %sel, %4
+  br i1 %matches, label %catch, label %eh.resume
+
+catch:; preds = %catch.dispatch
+  %exn = load i8*, i8** %exn.slot, align 4
+  %5 = call i8* @__cxa_begin_catch(i8* %exn) #2
+  %6 = bitcast i8* %5 to i32*
+  %7 = load i32, i32* %6, align 4
+  store i32 %7, i32* %0, align 4
+  store i32 2, i32* %retval, align 4
+  call void @__cxa_end_catch() #2
+  br label %return
+
+try.cont: ; preds = %invoke.cont
+  store i32 1, i32* %retval, align 4
+  br label %return
+
+return:   ; preds = %try.cont, %catch
+  %8 = load i32, i32* %retval, align 4
+  ret i32 %8
+
+eh.resume:; preds = %catch.dispatch
+  %exn1 = load i8*, i8** %exn.slot, align 4
+  %sel2 = load i32, i32* %ehselector.slot, align 4
+  %lpad.val = insertvalue { i8*, i32 } undef, i8* %exn1, 0
+  %lpad.val3 = insertvalue { i8*, i32 } %lpad.val, i32 %sel2, 1
+  resume { i8*, i32 } %lpad.val3
+}
+
+; ASM:  ._Z9catchFuncv:
+; ASM:  L..func_begin0:
+; ASM:  # %bb.0:# %entry
+; ASM:  	mflr 0
+; ASM:  L..tmp0:
+; ASM:  	bl ._Z9throwFuncv
+; ASM:  	nop
+; ASM:  L..tmp1:
+; ASM:  # %bb.1:# %invoke.cont
+; ASM:  	li 3, 1
+; ASM:  L..BB1_2:   # %return
+; ASM:  	mtlr 0
+; ASM:  	blr
+; ASM:  L..BB1_3:   # %lpad
+; ASM:  L..tmp2:
+; ASM:  	bl .__cxa_begin_catch[PR]
+; ASM:  	nop
+; ASM:  	bl .__cxa_end_catch[PR]
+; ASM:  	nop
+; ASM:  	b L..BB1_2
+; ASM:  L..func_end0:
+
+; ASM:  	.csect .gcc_except_table[RO],2
+; ASM:  	.align	2
+; ASM:  GCC_except_table1:
+; ASM:  L..exception0:
+; ASM:  	.byte	255 # @LPStart Encoding = omit
+; ASM32:	.byte	187 # @TType Encoding = 
+; ASM64:  

[PATCH] D91455: [XCOFF][AIX] Generate LSDA data and compact unwind section on AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu added a comment.

In D91455#2424584 , 
@hubert.reinterpretcast wrote:

>> 2. AIX uses a new personality routine, named __xlcxx_personality_v1. It 
>> doesn't use the GCC personality rountine, because the intractability is not 
>> there yet on AIX.
>
> @jasonliu, is "intractability" a typo/autocorrect problem?

Yep. Fixed!


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

https://reviews.llvm.org/D91455

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

[PATCH] D91455: [XCOFF][AIX] Generate LSDA data and compact unwind section on AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:889
+  CompactUnwindSection =
+  Ctx->getXCOFFSection(".eh_info_table", 
XCOFF::StorageMappingClass::XMC_RW,
+   XCOFF::XTY_SD, SectionKind::getData());

daltenty wrote:
> jasonliu wrote:
> > daltenty wrote:
> > > I think this may have been discussed elsewhere, but why do we want to 
> > > emit this as a RW section?
> > Yes, this has been discussed before. And it's a good question.
> > If we emit this as an RO section, then we could not put 
> > ```
> > .vbyte  4, GCC_except_table1
> > .vbyte  4, __xlcxx_personality_v1[DS]
> > ```
> > into this csect directly. 
> > If we do that, we would hit linker error during link time. It seems that 
> > for read only csects (RO, PR mapping classes), we could not have relocation 
> > types like `R_POS` or `R_NEG` because the result coming out of it is not 
> > link-time constant. And the relocation types in read only csects needs to 
> > be link time constant.
> > 
> > Extra work (both compiler and runtime) will be needed if we want to make 
> > this RO.
> Thanks for clarifying. I guess we'd presumably need to add the TOC 
> indirections to get to the LSDA and personality routine's function 
> descriptor,  similar  to elsewhere. 
> 
> Since this structure is version'd, seems like we have the flexibility to 
> defer that work, so I guess that's OK.
> add the TOC indirections to get to the LSDA and personality routine's 
> function descriptor
Yep, that's one potential way to solve it. The downside of it would be 
1. We might have more TC entries than we desired.
2. A small performance penalty to pay for the extra indirection. (But since we 
already on the EH path, and it's slow anyway, might not be that big of a deal).
But as you already mentioned, we don't have to solve this issue right now. 


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

https://reviews.llvm.org/D91455

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

[PATCH] D91455: [XCOFF][AIX] Generate LSDA data and compact unwind section on AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:889
+  CompactUnwindSection =
+  Ctx->getXCOFFSection(".eh_info_table", 
XCOFF::StorageMappingClass::XMC_RW,
+   XCOFF::XTY_SD, SectionKind::getData());

daltenty wrote:
> I think this may have been discussed elsewhere, but why do we want to emit 
> this as a RW section?
Yes, this has been discussed before. And it's a good question.
If we emit this as an RO section, then we could not put 
```
.vbyte  4, GCC_except_table1
.vbyte  4, __xlcxx_personality_v1[DS]
```
into this csect directly. 
If we do that, we would hit linker error during link time. It seems that for 
read only csects (RO, PR mapping classes), we could not have relocation types 
like `R_POS` or `R_NEG` because the result coming out of it is not link-time 
constant. And the relocation types in read only csects needs to be link time 
constant.

Extra work (both compiler and runtime) will be needed if we want to make this 
RO.


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

https://reviews.llvm.org/D91455

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

[PATCH] D91455: [XCOFF][AIX] Generate LSDA data and compact unwind section on AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu created this revision.
jasonliu added reviewers: xingxue, hubert.reinterpretcast, cebowleratibm, 
DiggerLin, daltenty.
Herald added subscribers: llvm-commits, kbarton, hiraditya, mgorny, nemanjai.
Herald added a project: LLVM.
jasonliu requested review of this revision.
Herald added a subscriber: aheejin.

AIX uses the existing EH infrastructure in clang and llvm.
The major differences would be

1. AIX do not have CFI instructions.
2. AIX uses a new personality routine, named __xlcxx_personality_v1. It doesn't 
use the GCC personality rountine, because the intractability is not there yet 
on AIX.
3. AIX do not use eh_frame sections.  Instead, it would use a eh_info section 
(compat unwind section) to store the information about personality routine and 
LSDA data address.


https://reviews.llvm.org/D91455

Files:
  clang/lib/CodeGen/CGCleanup.h
  clang/lib/CodeGen/CGException.cpp
  clang/test/CodeGenCXX/personality.cpp
  llvm/include/llvm/Analysis/EHPersonalities.h
  llvm/include/llvm/CodeGen/AsmPrinter.h
  llvm/include/llvm/MC/MCTargetOptions.h
  llvm/lib/Analysis/EHPersonalities.cpp
  llvm/lib/CodeGen/AsmPrinter/AIXException.cpp
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
  llvm/lib/CodeGen/AsmPrinter/CMakeLists.txt
  llvm/lib/CodeGen/AsmPrinter/DwarfException.h
  llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/MC/MCAsmInfoXCOFF.cpp
  llvm/lib/MC/MCObjectFileInfo.cpp
  llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/test/CodeGen/PowerPC/aix-exception.ll

Index: llvm/test/CodeGen/PowerPC/aix-exception.ll
===
--- /dev/null
+++ llvm/test/CodeGen/PowerPC/aix-exception.ll
@@ -0,0 +1,152 @@
+; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec < %s | \
+; RUN:   FileCheck --check-prefixes=ASM,ASM32 %s
+
+; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec < %s | \
+; RUN:   FileCheck --check-prefixes=ASM,ASM64 %s
+
+@_ZTIi = external constant i8*
+
+define void @_Z9throwFuncv() {
+entry:
+  %exception = call i8* @__cxa_allocate_exception(i32 4) #2
+  %0 = bitcast i8* %exception to i32*
+  store i32 1, i32* %0, align 16
+  call void @__cxa_throw(i8* %exception, i8* bitcast (i8** @_ZTIi to i8*), i8* null) #3
+  unreachable
+}
+
+; ASM:._Z9throwFuncv:
+; ASM:  bl .__cxa_allocate_exception[PR]
+; ASM:  nop
+; ASM32:lwz 4, L..C0(2)
+; ASM64:ld 4, L..C0(2)
+; ASM:  bl .__cxa_throw[PR]
+; ASM:  nop
+
+define i32 @_Z9catchFuncv() personality i8* bitcast (i32 (...)* @__xlcxx_personality_v1 to i8*) {
+entry:
+  %retval = alloca i32, align 4
+  %exn.slot = alloca i8*, align 4
+  %ehselector.slot = alloca i32, align 4
+  %0 = alloca i32, align 4
+  invoke void @_Z9throwFuncv()
+  to label %invoke.cont unwind label %lpad
+
+invoke.cont:  ; preds = %entry
+  br label %try.cont
+
+lpad: ; preds = %entry
+  %1 = landingpad { i8*, i32 }
+  catch i8* bitcast (i8** @_ZTIi to i8*)
+  %2 = extractvalue { i8*, i32 } %1, 0
+  store i8* %2, i8** %exn.slot, align 4
+  %3 = extractvalue { i8*, i32 } %1, 1
+  store i32 %3, i32* %ehselector.slot, align 4
+  br label %catch.dispatch
+
+catch.dispatch:   ; preds = %lpad
+  %sel = load i32, i32* %ehselector.slot, align 4
+  %4 = call i32 @llvm.eh.typeid.for(i8* bitcast (i8** @_ZTIi to i8*)) #2
+  %matches = icmp eq i32 %sel, %4
+  br i1 %matches, label %catch, label %eh.resume
+
+catch:; preds = %catch.dispatch
+  %exn = load i8*, i8** %exn.slot, align 4
+  %5 = call i8* @__cxa_begin_catch(i8* %exn) #2
+  %6 = bitcast i8* %5 to i32*
+  %7 = load i32, i32* %6, align 4
+  store i32 %7, i32* %0, align 4
+  store i32 2, i32* %retval, align 4
+  call void @__cxa_end_catch() #2
+  br label %return
+
+try.cont: ; preds = %invoke.cont
+  store i32 1, i32* %retval, align 4
+  br label %return
+
+return:   ; preds = %try.cont, %catch
+  %8 = load i32, i32* %retval, align 4
+  ret i32 %8
+
+eh.resume:; preds = %catch.dispatch
+  %exn1 = load i8*, i8** %exn.slot, align 4
+  %sel2 = load i32, i32* %ehselector.slot, align 4
+  %lpad.val = insertvalue { i8*, i32 } undef, i8* %exn1, 0
+  %lpad.val3 = insertvalue { i8*, i32 } %lpad.val, i32 %sel2, 1
+  resume { i8*, i32 } %lpad.val3
+}
+
+; ASM:  ._Z9catchFuncv:
+; ASM:  L..func_begin0:
+; ASM:  # %bb.0:# %entry
+; ASM:  	mflr 0
+; ASM:  L..tmp0:
+; ASM:  	bl ._Z9throwFuncv
+; ASM:  	nop
+; ASM:  L..tmp1:
+; ASM:  # %bb.1: 

[PATCH] D89986: [AIX] do not emit visibility attribute into IR when there is -mignore-xcoff-visibility

[Jason Liu via Phabricator via cfe-commits]

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

LGTM with minor nit.




Comment at: clang/lib/AST/Decl.cpp:1481
 LinkageInfo LinkageComputer::getDeclLinkageAndVisibility(const NamedDecl *D) {
-  return getLVForDecl(D,
-  LVComputationKind(usesTypeVisibility(D)
-? NamedDecl::VisibilityForType
-: NamedDecl::VisibilityForValue));
+  NamedDecl::ExplicitVisibilityKind EK = usesTypeVisibility(D)
+ ? NamedDecl::VisibilityForType

`clang-format` this please.
This line seems to exceed 80 columns. 



Comment at: clang/test/CodeGen/aix-visibility-inlines-hidden.cpp:1
+// REQUIRES: powerpc-registered-target
+

Do you need this line?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

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

[PATCH] D89986: [AIX] do not emit visibility attribute into IR when there is -mignore-xcoff-visibility

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:520
   Options.DataSections = CodeGenOpts.DataSections;
-  Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;

DiggerLin wrote:
> DiggerLin wrote:
> > jasonliu wrote:
> > > DiggerLin wrote:
> > > > jasonliu wrote:
> > > > > DiggerLin wrote:
> > > > > > jasonliu wrote:
> > > > > > > Instead of just removing this line, should this get replaced with 
> > > > > > > the new LangOpts option?
> > > > > > I do not think we need a CodeGenOp of ignore-xcoff-visibility in 
> > > > > > clang, we only need the LangOpt of the ignore-xcoff-visilbity to 
> > > > > > control whether we will  generate the visibility in the IR,  when 
> > > > > > the LangOpt of ignore-xcoff-visibility do not generate the 
> > > > > > visibility attribute of GV in the IR. it do not need CodeGenOp of 
> > > > > > ignore-xcoff-visibility any more for the clang .
> > > > > > 
> > > > > > we have still CodeGen ignore-xcoff-visibility op in  llc.
> > > > > We removed the visibility from IR level with this patch. But there is 
> > > > > also visibility settings coming from CodeGen part of clang, which 
> > > > > needs to get ignore when we are doing the code gen in llc. So I think 
> > > > > you still need to set the options correct for llc.
> > > > yes we have the set the options correct for llc in the code.
> > > > 
> > > > in the source file llvm/lib/CodeGen/CommandFlags.cpp, we have (in the 
> > > > patch https://reviews.llvm.org/D87451 add new option 
> > > > -mignore-xcoff-visibility) , the function
> > > > TargetOptions codegen::InitTargetOptionsFromCodeGenFlags() {
> > > > 
> > > > Options.IgnoreXCOFFVisibility = getIgnoreXCOFFVisibility(); 
> > > > ...}
> > > > 
> > > What I'm saying is... 
> > > I think we need a line like this:
> > > `Options.IgnoreXCOFFVisibility = LangOpts.IgnoreXCOFFVisibility;`
> > > so that when you invoke clang, backend would get the correct setting as 
> > > well. 
> > I do not think so, from the clang FE, we do not generated the visibility in 
> > the IR. so there is no need these line.
> or we can say that because we do not set the hidden visibility into the 
> GlobalValue , so we do not need the 
> Options.IgnoreXCOFFVisibility = LangOpts.IgnoreXCOFFVisibility;
I think I mentioned this before, we could have extra visibility settings in 
clang/lib/CodeGen that's not depending on the existing visibility setting in 
the IR. (You could search for `setVisibility` there.) That was the reason we 
did it in llc first. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

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

[PATCH] D89986: [AIX] do not emit visibility attribute into IR when there is -mignore-xcoff-visibility

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:520
   Options.DataSections = CodeGenOpts.DataSections;
-  Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;

DiggerLin wrote:
> jasonliu wrote:
> > DiggerLin wrote:
> > > jasonliu wrote:
> > > > Instead of just removing this line, should this get replaced with the 
> > > > new LangOpts option?
> > > I do not think we need a CodeGenOp of ignore-xcoff-visibility in clang, 
> > > we only need the LangOpt of the ignore-xcoff-visilbity to control whether 
> > > we will  generate the visibility in the IR,  when the LangOpt of 
> > > ignore-xcoff-visibility do not generate the visibility attribute of GV in 
> > > the IR. it do not need CodeGenOp of ignore-xcoff-visibility any more for 
> > > the clang .
> > > 
> > > we have still CodeGen ignore-xcoff-visibility op in  llc.
> > We removed the visibility from IR level with this patch. But there is also 
> > visibility settings coming from CodeGen part of clang, which needs to get 
> > ignore when we are doing the code gen in llc. So I think you still need to 
> > set the options correct for llc.
> yes we have the set the options correct for llc in the code.
> 
> in the source file llvm/lib/CodeGen/CommandFlags.cpp, we have (in the patch 
> https://reviews.llvm.org/D87451 add new option -mignore-xcoff-visibility) , 
> the function
> TargetOptions codegen::InitTargetOptionsFromCodeGenFlags() {
> 
> Options.IgnoreXCOFFVisibility = getIgnoreXCOFFVisibility(); 
> ...}
> 
What I'm saying is... 
I think we need a line like this:
`Options.IgnoreXCOFFVisibility = LangOpts.IgnoreXCOFFVisibility;`
so that when you invoke clang, backend would get the correct setting as well. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

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

[PATCH] D89986: [AIX] do not emit visibility attribute into IR when there is -mignore-xcoff-visibility

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:520
   Options.DataSections = CodeGenOpts.DataSections;
-  Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;

DiggerLin wrote:
> jasonliu wrote:
> > Instead of just removing this line, should this get replaced with the new 
> > LangOpts option?
> I do not think we need a CodeGenOp of ignore-xcoff-visibility in clang, we 
> only need the LangOpt of the ignore-xcoff-visilbity to control whether we 
> will  generate the visibility in the IR,  when the LangOpt of 
> ignore-xcoff-visibility do not generate the visibility attribute of GV in the 
> IR. it do not need CodeGenOp of ignore-xcoff-visibility any more for the 
> clang .
> 
> we have still CodeGen ignore-xcoff-visibility op in  llc.
We removed the visibility from IR level with this patch. But there is also 
visibility settings coming from CodeGen part of clang, which needs to get 
ignore when we are doing the code gen in llc. So I think you still need to set 
the options correct for llc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

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

[PATCH] D89986: [AIX] do not emit visibility attribute into IR when there is -mignore-xcoff-visibility

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:520
   Options.DataSections = CodeGenOpts.DataSections;
-  Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;

Instead of just removing this line, should this get replaced with the new 
LangOpts option?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

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

[PATCH] D89897: [AIX] Emit error for -G option on AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu added a comment.

Could we add some rationale to the patch summary about why we would want to 
deliberately emitting an error for AIX?




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4924
 
+  if (RawTriple.isOSAIX())
+if (Arg *A = Args.getLastArg(options::OPT_G)) {

Question: When do we query `RawTriple`, and when we should query `Triple`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89897

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

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

[Jason Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf85bcc21ddad: [AIX] Turn -fdata-sections on by default in 
Clang (authored by jasonliu).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D88737?vs=296965=298160#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88737

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/aix-data-sections.c
  clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
  lld/Common/TargetOptionsCommandFlags.cpp
  llvm/include/llvm/ADT/Triple.h
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/test/CodeGen/PowerPC/aix-alias.ll
  llvm/test/CodeGen/PowerPC/aix-bytestring.ll
  llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
  llvm/test/CodeGen/PowerPC/aix-extern.ll
  llvm/test/CodeGen/PowerPC/aix-ignore-xcoff-visibility.ll
  llvm/test/CodeGen/PowerPC/aix-overflow-toc.py
  llvm/test/CodeGen/PowerPC/aix-readonly-with-relocation.ll
  llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll
  llvm/test/CodeGen/PowerPC/aix-return55.ll
  llvm/test/CodeGen/PowerPC/aix-weak.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-data-sections.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-lower-comm.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-rodata.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-symbol-rename.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-used.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-visibility.ll
  llvm/tools/gold/gold-plugin.cpp
  llvm/tools/llc/llc.cpp
  llvm/tools/lli/lli.cpp
  llvm/tools/llvm-isel-fuzzer/llvm-isel-fuzzer.cpp
  llvm/tools/llvm-lto/llvm-lto.cpp
  llvm/tools/llvm-lto2/llvm-lto2.cpp
  llvm/tools/llvm-opt-fuzzer/llvm-opt-fuzzer.cpp
  llvm/tools/lto/lto.cpp
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -698,7 +698,8 @@
   Triple ModuleTriple(M->getTargetTriple());
   std::string CPUStr, FeaturesStr;
   TargetMachine *Machine = nullptr;
-  const TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  const TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(ModuleTriple);
 
   if (ModuleTriple.getArch()) {
 CPUStr = codegen::getCPUStr();
Index: llvm/tools/lto/lto.cpp
===
--- llvm/tools/lto/lto.cpp
+++ llvm/tools/lto/lto.cpp
@@ -218,7 +218,8 @@
 
 lto_module_t lto_module_create(const char* path) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M =
   LTOModule::createFromFile(*LTOContext, StringRef(path), Options);
   if (!M)
@@ -228,7 +229,8 @@
 
 lto_module_t lto_module_create_from_fd(int fd, const char *path, size_t size) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromOpenFile(
   *LTOContext, fd, StringRef(path), size, Options);
   if (!M)
@@ -241,7 +243,8 @@
  size_t map_size,
  off_t offset) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromOpenFileSlice(
   *LTOContext, fd, StringRef(path), map_size, offset, Options);
   if (!M)
@@ -251,7 +254,8 @@
 
 lto_module_t lto_module_create_from_memory(const void* mem, size_t length) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M =
   LTOModule::createFromBuffer(*LTOContext, mem, length, Options);
   if (!M)
@@ -263,7 +267,8 @@
  size_t length,
  const char *path) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromBuffer(
   *LTOContext, mem, length, Options, StringRef(path));
   if (!M)
@@ -274,7 +279,8 @@
 lto_module_t lto_module_create_in_local_context(const void *mem, size_t length,
 

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

[Jason Liu via Phabricator via cfe-commits]

jasonliu added a comment.

Hi Fangrui(@MaskRay), are you okay with this patch to land as is?


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

https://reviews.llvm.org/D88737

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

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

[Jason Liu via Phabricator via cfe-commits]

jasonliu added a comment.

In D88737#2321921 , @MaskRay wrote:

> A less intrusive approach is to not touch the existing 
> InitTargetOptionsFromCodeGenFlags without parameters. You can add an 
> `initTargetOptionsFromCodeGenFlags` with `const Tripe &` as its parameter.

I thought about that. But would prefer people think about whether or not a 
proper Triple is actually needed in that scenario before they make a call to 
`InitTargetOptionsFromCodeGenFlags`. 
Have an `InitTargetOptionsFromCodeGenFlags` without parameter, or has Triple() 
as default parameter could potentially results in people choose the wrong one 
to call because it's just more convenient.


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

https://reviews.llvm.org/D88737

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/include/clang/Basic/LangOptions.def:340
 
+LANGOPT(AIXPragmaPack, 1, 0, "AIX #pragma pack handling")
+

Not sure if AIXPragmaPack is the best name here. 
It's more like IBM xl pragma pack handling on AIX.
Would it be better if we name it `XLPragmaPackOnAIX`?



Comment at: clang/include/clang/Sema/Sema.h:493
+  PackNumber(M == Packed ? 1
+ : (M == Mac68k ? Mac68kAlignmentSentinel
+: UninitPackVal)),

I think one of the idea is to use `enum Mode::Mac68k` to replace the need of 
Mac68kAlignmentSentinel.
Is there any reason that you would still need PackNumber to contain 
`Mac68kAlignmentSentinel`?



Comment at: clang/include/clang/Sema/Sema.h:497
+
+AlignPackInfo() : AlignPackInfo(Native, false) {}
+

We could have a empty stack on AIX that returns false when it was asked if it's 
an AIX stack?
It's a bit confusing here. 



Comment at: clang/include/clang/Sema/Sema.h:637
   PragmaStack VtorDispStack;
-  // #pragma pack.
-  // Sentinel to represent when the stack is set to mac68k alignment.
-  static const unsigned kMac68kAlignmentSentinel = ~0U;
-  PragmaStack PackStack;
+  PragmaStack PackStack;
   // The current #pragma pack values and locations at each #include.

We should consider renaming PackStack to AlignPackStack across Clang. Maybe 
even as a NFC first. As it is right now, clang already uses one stack to record 
those two informations from `Pragma align` and `Pragma pack`. Leave it as 
PackStack is too confusing, and people could actually ignore the pragma Align 
when developing with PackStack. 



Comment at: clang/include/clang/Sema/Sema.h:488
+AlignPackInfo(AlignPackInfo::Mode M, int Num, bool IsAIX)
+: PackAttr(true), AlignMode(M), PackNumber(Num), AIXStack(IsAIX) {}
+

Xiangling_L wrote:
> jasonliu wrote:
> > I noticed PackNumber is an unsigned char and we are passing an int type 
> > into it.
> > Could we add an assertion in the constructor to make sure Num would never 
> > be something that's going to get truncated when it converts to an unsigned 
> > char?
> I think the warning/error `expected #pragma pack parameter to be '1', '2', 
> '4', '8', or '16'` have already guaranteed that for us? Or maybe using 
> `unsigned int` makes people more comfortable?
Using a char instead of an int could make this class more compact, especially 
when we actually don't need to represent a big range. 
I understand there is a warning/error somewhere to prevent out of range from 
happening. But an assert here would make sure that no one could pass in an 
unexpected value into here by accident when developing. 



Comment at: clang/include/clang/Sema/Sema.h:515
+bool operator==(AlignPackInfo Info) const {
+  return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
+}

Xiangling_L wrote:
> jasonliu wrote:
> > This could return true when `PackAttr` in AlignPackInfo are not the same. 
> > Wouldn't that cause an issue?
> (1) You mean we have two `AlignPackInfo` with same AlignMode and PackNumber, 
> but one is PackAttr and the other one is AlignAttr?
> The example I can think of is:
> 
> 
> ```
> a)#pragma align(packed)
>   #pragma pack(1)   //AlignMode = Packed, PackNumber = 1
> 
> b) #pragma align(packed)  //AlignMode = Packed, PackNumber = 1
> ```
> 
> But I don't think we have any issue in this case. Before and after my patch, 
> a == b.
> Please let me know any other cases concerning you if any.
> 
> (2) However, your concerns leads me to think of another case, where behavior 
> changes with my patch.
> 
> ```
> a) 
> #pragma align(natural)
> #pragma pack(1)   /AlignMode = Native,  PackNumber = 1
> 
> b)
> #pragma align(packed) ///AlignMode = Packed, PackNumber = 1
> 
> ```
> Without this patch, a == b for other targets.
> And I suppose a != b for AIX.
> 
In your first example, if I understand correctly,
a) would return true for IsPackAttr()
b) would return false for IsPackAttr()
and yet a == b ?
I think that's confusing. 

Any reason why you don't want to just compare all the data members to make sure 
they are all equal?



Comment at: clang/lib/Sema/SemaAttr.cpp:367
+  // AIX pragma pack does not support identifier syntax.
+  if (getLangOpts().AIXPragmaPack && !SlotLabel.empty()) {
+Diag(PragmaLoc, diag::warn_pragma_pack_identifer_not_supported);

Although IBM xl compiler does not support this form, do we see a harm for us to 
support this form in clang on AIX?
Also, if this is indeed not desired to support, we could move this check to the 
top of this function for an early return. 



Comment at: clang/lib/Sema/SemaAttr.cpp:403
   // Warn about modified alignment after #includes.
   if 

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: lld/Common/TargetOptionsCommandFlags.cpp:17
+llvm::TargetOptions
+lld::initTargetOptionsFromCodeGenFlags(const llvm::Triple ) {
+  return llvm::codegen::InitTargetOptionsFromCodeGenFlags(TheTriple);

MaskRay wrote:
> jasonliu wrote:
> > MaskRay wrote:
> > > Currently lld does not need Triple. Consider not changing the signature 
> > > of `initTargetOptionsFromCodeGenFlags`.
> > This function acts like a forwarder for 
> > llvm::codegen::InitTargetOptionsFromCodeGenFlags.
> > I think it still makes sense to change the signature in this case to 
> > minimize the different variation of the function, as those variations cause 
> > confusion to people. 
> > I will change the name to match the LLVM style.
> If you want to change `InitTargetOptionsFromCodeGenFlags` in an incompatible 
> way, you can by the way rename it to `initTarget*`.
> 
> lld functions should always stick with the `camelCase` rule. If there is no 
> meaningful triple, I'd prefer leave out the parameter.
Sorry, didn't realize lld has this `camelCase` rule. 
I don't have a better naming for it, so I will just leave the function 
signature untouched. 


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

https://reviews.llvm.org/D88737

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

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 296965.

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

https://reviews.llvm.org/D88737

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/aix-data-sections.c
  clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
  lld/Common/TargetOptionsCommandFlags.cpp
  llvm/include/llvm/ADT/Triple.h
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/test/CodeGen/PowerPC/aix-alias.ll
  llvm/test/CodeGen/PowerPC/aix-bytestring.ll
  llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
  llvm/test/CodeGen/PowerPC/aix-extern.ll
  llvm/test/CodeGen/PowerPC/aix-overflow-toc.py
  llvm/test/CodeGen/PowerPC/aix-readonly-with-relocation.ll
  llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll
  llvm/test/CodeGen/PowerPC/aix-return55.ll
  llvm/test/CodeGen/PowerPC/aix-weak.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-data-sections.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-lower-comm.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-rodata.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-symbol-rename.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-used.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-visibility.ll
  llvm/tools/gold/gold-plugin.cpp
  llvm/tools/llc/llc.cpp
  llvm/tools/lli/lli.cpp
  llvm/tools/llvm-isel-fuzzer/llvm-isel-fuzzer.cpp
  llvm/tools/llvm-lto/llvm-lto.cpp
  llvm/tools/llvm-lto2/llvm-lto2.cpp
  llvm/tools/llvm-opt-fuzzer/llvm-opt-fuzzer.cpp
  llvm/tools/lto/lto.cpp
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -698,7 +698,8 @@
   Triple ModuleTriple(M->getTargetTriple());
   std::string CPUStr, FeaturesStr;
   TargetMachine *Machine = nullptr;
-  const TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  const TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(ModuleTriple);
 
   if (ModuleTriple.getArch()) {
 CPUStr = codegen::getCPUStr();
Index: llvm/tools/lto/lto.cpp
===
--- llvm/tools/lto/lto.cpp
+++ llvm/tools/lto/lto.cpp
@@ -218,7 +218,8 @@
 
 lto_module_t lto_module_create(const char* path) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M =
   LTOModule::createFromFile(*LTOContext, StringRef(path), Options);
   if (!M)
@@ -228,7 +229,8 @@
 
 lto_module_t lto_module_create_from_fd(int fd, const char *path, size_t size) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromOpenFile(
   *LTOContext, fd, StringRef(path), size, Options);
   if (!M)
@@ -241,7 +243,8 @@
  size_t map_size,
  off_t offset) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromOpenFileSlice(
   *LTOContext, fd, StringRef(path), map_size, offset, Options);
   if (!M)
@@ -251,7 +254,8 @@
 
 lto_module_t lto_module_create_from_memory(const void* mem, size_t length) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M =
   LTOModule::createFromBuffer(*LTOContext, mem, length, Options);
   if (!M)
@@ -263,7 +267,8 @@
  size_t length,
  const char *path) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromBuffer(
   *LTOContext, mem, length, Options, StringRef(path));
   if (!M)
@@ -274,7 +279,8 @@
 lto_module_t lto_module_create_in_local_context(const void *mem, size_t length,
 const char *path) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
 
   // Create a local context. Ownership will be transferred to LTOModule.
   std::unique_ptr Context = std::make_unique();

[PATCH] D87451: add new option -mignore-xcoff-visibility

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: llvm/include/llvm/Target/TargetOptions.h:126
   FunctionSections(false), DataSections(false),
-  UniqueSectionNames(true), UniqueBasicBlockSectionNames(false),
-  TrapUnreachable(false), NoTrapAfterNoreturn(false), TLSSize(0),
-  EmulatedTLS(false), ExplicitEmulatedTLS(false), EnableIPRA(false),
+  IgnoreXCOFFVisibility(false), UniqueSectionNames(true),
+  UniqueBasicBlockSectionNames(false), TrapUnreachable(false),

Should the default be true for this option?
As this is an XCOFF only option, and the default for clang is true, so it would 
be better for llc to match as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87451

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

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 296665.
jasonliu added a comment.

Change lld's forwarder function's signature to match LLVM style.


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

https://reviews.llvm.org/D88737

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/aix-data-sections.c
  clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
  lld/COFF/LTO.cpp
  lld/Common/TargetOptionsCommandFlags.cpp
  lld/ELF/LTO.cpp
  lld/include/lld/Common/TargetOptionsCommandFlags.h
  lld/wasm/LTO.cpp
  llvm/include/llvm/ADT/Triple.h
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/test/CodeGen/PowerPC/aix-alias.ll
  llvm/test/CodeGen/PowerPC/aix-bytestring.ll
  llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
  llvm/test/CodeGen/PowerPC/aix-extern.ll
  llvm/test/CodeGen/PowerPC/aix-overflow-toc.py
  llvm/test/CodeGen/PowerPC/aix-readonly-with-relocation.ll
  llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll
  llvm/test/CodeGen/PowerPC/aix-return55.ll
  llvm/test/CodeGen/PowerPC/aix-weak.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-data-sections.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-lower-comm.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-rodata.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-symbol-rename.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-used.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-visibility.ll
  llvm/tools/gold/gold-plugin.cpp
  llvm/tools/llc/llc.cpp
  llvm/tools/lli/lli.cpp
  llvm/tools/llvm-isel-fuzzer/llvm-isel-fuzzer.cpp
  llvm/tools/llvm-lto/llvm-lto.cpp
  llvm/tools/llvm-lto2/llvm-lto2.cpp
  llvm/tools/llvm-opt-fuzzer/llvm-opt-fuzzer.cpp
  llvm/tools/lto/lto.cpp
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -698,7 +698,8 @@
   Triple ModuleTriple(M->getTargetTriple());
   std::string CPUStr, FeaturesStr;
   TargetMachine *Machine = nullptr;
-  const TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  const TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(ModuleTriple);
 
   if (ModuleTriple.getArch()) {
 CPUStr = codegen::getCPUStr();
Index: llvm/tools/lto/lto.cpp
===
--- llvm/tools/lto/lto.cpp
+++ llvm/tools/lto/lto.cpp
@@ -218,7 +218,8 @@
 
 lto_module_t lto_module_create(const char* path) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M =
   LTOModule::createFromFile(*LTOContext, StringRef(path), Options);
   if (!M)
@@ -228,7 +229,8 @@
 
 lto_module_t lto_module_create_from_fd(int fd, const char *path, size_t size) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromOpenFile(
   *LTOContext, fd, StringRef(path), size, Options);
   if (!M)
@@ -241,7 +243,8 @@
  size_t map_size,
  off_t offset) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromOpenFileSlice(
   *LTOContext, fd, StringRef(path), map_size, offset, Options);
   if (!M)
@@ -251,7 +254,8 @@
 
 lto_module_t lto_module_create_from_memory(const void* mem, size_t length) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M =
   LTOModule::createFromBuffer(*LTOContext, mem, length, Options);
   if (!M)
@@ -263,7 +267,8 @@
  size_t length,
  const char *path) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromBuffer(
   *LTOContext, mem, length, Options, StringRef(path));
   if (!M)
@@ -274,7 +279,8 @@
 lto_module_t lto_module_create_in_local_context(const void *mem, size_t length,
 const char *path) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

[Jason Liu via Phabricator via cfe-commits]

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



Comment at: clang/test/Driver/aix-data-sections.c:7
+// RUN:   | FileCheck %s
+// CHECK: "-fdata-sections"

DiggerLin wrote:
> may be good to check the whether other OS platform behavior be changed or 
> not? 
I think there are existing lit tests checked other platform's behavior.

linux platform behavior is tested in clang/test/Driver/function-sections.c
cloudABI platform behavior is tested in clang/test/Driver/cloudabi.c
msvc behavior is tested in clang/test/Driver/cl-options.c



Comment at: lld/Common/TargetOptionsCommandFlags.cpp:17
+llvm::TargetOptions
+lld::initTargetOptionsFromCodeGenFlags(const llvm::Triple ) {
+  return llvm::codegen::InitTargetOptionsFromCodeGenFlags(TheTriple);

MaskRay wrote:
> Currently lld does not need Triple. Consider not changing the signature of 
> `initTargetOptionsFromCodeGenFlags`.
This function acts like a forwarder for 
llvm::codegen::InitTargetOptionsFromCodeGenFlags.
I think it still makes sense to change the signature in this case to minimize 
the different variation of the function, as those variations cause confusion to 
people. 
I will change the name to match the LLVM style.


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

https://reviews.llvm.org/D88737

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

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 295950.
jasonliu added a comment.
Herald added subscribers: llvm-commits, nemanjai.
Herald added a project: LLVM.

An update follows up with the idea in D88748 .


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

https://reviews.llvm.org/D88737

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/aix-data-sections.c
  clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
  lld/COFF/LTO.cpp
  lld/Common/TargetOptionsCommandFlags.cpp
  lld/ELF/LTO.cpp
  lld/include/lld/Common/TargetOptionsCommandFlags.h
  lld/wasm/LTO.cpp
  llvm/include/llvm/ADT/Triple.h
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/test/CodeGen/PowerPC/aix-alias.ll
  llvm/test/CodeGen/PowerPC/aix-bytestring.ll
  llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
  llvm/test/CodeGen/PowerPC/aix-extern.ll
  llvm/test/CodeGen/PowerPC/aix-overflow-toc.py
  llvm/test/CodeGen/PowerPC/aix-readonly-with-relocation.ll
  llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll
  llvm/test/CodeGen/PowerPC/aix-return55.ll
  llvm/test/CodeGen/PowerPC/aix-weak.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-data-sections.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-lower-comm.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-rodata.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-symbol-rename.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-used.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-visibility.ll
  llvm/tools/gold/gold-plugin.cpp
  llvm/tools/llc/llc.cpp
  llvm/tools/lli/lli.cpp
  llvm/tools/llvm-isel-fuzzer/llvm-isel-fuzzer.cpp
  llvm/tools/llvm-lto/llvm-lto.cpp
  llvm/tools/llvm-lto2/llvm-lto2.cpp
  llvm/tools/llvm-opt-fuzzer/llvm-opt-fuzzer.cpp
  llvm/tools/lto/lto.cpp
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -698,7 +698,8 @@
   Triple ModuleTriple(M->getTargetTriple());
   std::string CPUStr, FeaturesStr;
   TargetMachine *Machine = nullptr;
-  const TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  const TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(ModuleTriple);
 
   if (ModuleTriple.getArch()) {
 CPUStr = codegen::getCPUStr();
Index: llvm/tools/lto/lto.cpp
===
--- llvm/tools/lto/lto.cpp
+++ llvm/tools/lto/lto.cpp
@@ -218,7 +218,8 @@
 
 lto_module_t lto_module_create(const char* path) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M =
   LTOModule::createFromFile(*LTOContext, StringRef(path), Options);
   if (!M)
@@ -228,7 +229,8 @@
 
 lto_module_t lto_module_create_from_fd(int fd, const char *path, size_t size) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromOpenFile(
   *LTOContext, fd, StringRef(path), size, Options);
   if (!M)
@@ -241,7 +243,8 @@
  size_t map_size,
  off_t offset) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromOpenFileSlice(
   *LTOContext, fd, StringRef(path), map_size, offset, Options);
   if (!M)
@@ -251,7 +254,8 @@
 
 lto_module_t lto_module_create_from_memory(const void* mem, size_t length) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M =
   LTOModule::createFromBuffer(*LTOContext, mem, length, Options);
   if (!M)
@@ -263,7 +267,8 @@
  size_t length,
  const char *path) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromBuffer(
   *LTOContext, mem, length, Options, StringRef(path));
   if (!M)
@@ -274,7 +279,8 @@
 lto_module_t lto_module_create_in_local_context(const void *mem, size_t length,
 const char *path) {
   lto_initialize();
-  

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.def:47
+CODEGENOPT(DataSections  , 1, 0) ///< Set by default, or when 
-f[no-]data-sections.
+CODEGENOPT(HasExplicitDataSections, 1, 0) ///< Set when -f[no-]data-sections 
is set.
 CODEGENOPT(UniqueSectionNames, 1, 1) ///< Set for -funique-section-names.

jasonliu wrote:
> MaskRay wrote:
> > From the current code. I don't see HasExplicitDataSections is necessary. 
> > Please remove the lld changes.
> The reason I need `HasExplicitDataSections` goes back to D88493(which I 
> haven't land yet, because I actually need to land these two patches together 
> to avoid build break). In D88493, I'm trying to get the llc's default for 
> -data-sections to be correct for AIX and introduced 
> `HasExplicitDataSections`. If `HasExplicitDataSections` is not set, then llc 
> would think there is -data-sections set, so it would just take the target 
> triple's default, which makes `DataSections` setting to be useless. 
> It seems there is no good way to set a certain TargetOption's default to be 
> dependant on the current target triple. As I already mentioned in my own 
> comment in this patch, one of the way to achieve that could be make 
> DataSections an enum option in TargetOptions, so that it could have a 
> `Default` enum which could mean true or false depending on the triple 
> setting. But it could mean a bigger refactoring to this option. 
Correction:
If HasExplicitDataSections is not set, then llc would think there is **no** 
-data-sections set, so it would just take the target triple's default...


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

https://reviews.llvm.org/D88737

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

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.def:47
+CODEGENOPT(DataSections  , 1, 0) ///< Set by default, or when 
-f[no-]data-sections.
+CODEGENOPT(HasExplicitDataSections, 1, 0) ///< Set when -f[no-]data-sections 
is set.
 CODEGENOPT(UniqueSectionNames, 1, 1) ///< Set for -funique-section-names.

MaskRay wrote:
> From the current code. I don't see HasExplicitDataSections is necessary. 
> Please remove the lld changes.
The reason I need `HasExplicitDataSections` goes back to D88493(which I haven't 
land yet, because I actually need to land these two patches together to avoid 
build break). In D88493, I'm trying to get the llc's default for -data-sections 
to be correct for AIX and introduced `HasExplicitDataSections`. If 
`HasExplicitDataSections` is not set, then llc would think there is 
-data-sections set, so it would just take the target triple's default, which 
makes `DataSections` setting to be useless. 
It seems there is no good way to set a certain TargetOption's default to be 
dependant on the current target triple. As I already mentioned in my own 
comment in this patch, one of the way to achieve that could be make 
DataSections an enum option in TargetOptions, so that it could have a `Default` 
enum which could mean true or false depending on the triple setting. But it 
could mean a bigger refactoring to this option. 


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

https://reviews.llvm.org/D88737

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

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

[Jason Liu via Phabricator via cfe-commits]

jasonliu added a comment.

Not sure if there is a better way to do this. But I have to admit this approach 
along with D88493  is a bit fragile. 
If plug-in writer forgets to set `HasExplicitDataSections` to true, then the 
final result that TargetMachine give for `getDataSections()` could be false on 
most platform even when they deliberately set `DataSections` to true.
We have other targets in LLVM that have -fdata-sections by default as well. And 
they do it differently as well, which makes this a bit more complicate to be 
consistent. 
For example, cloudABI would pass -fdata-sections through the Clang Driver by 
default. But that approach means if some user just invoke llc directly, they 
could get the wrong default on llc for that platform. 
Wasm would just overwrite the `DataSections` option to true in their 
TargetMachine. But that means you could not set it to false even if you want to.
I've thought about making the `DataSections` option in TargetOptions an enum 
instead of boolean value, where you could have `Default`, `On` and `Off` to 
represent what we actually want for the options. That's not a trivial change, 
as a lot of consumers for TargetOptions are relying it to be a boolean.
If you think it's a better way to solve this problem and I should explore, let 
me know.


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

https://reviews.llvm.org/D88737

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

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

[Jason Liu via Phabricator via cfe-commits]

jasonliu created this revision.
jasonliu added reviewers: hubert.reinterpretcast, daltenty, sfertile, 
Xiangling_L, DiggerLin.
Herald added subscribers: dang, dexonsmith, steven_wu, hiraditya, arichardson, 
sbc100, emaste.
Herald added a reviewer: espindola.
Herald added a reviewer: MaskRay.
jasonliu requested review of this revision.
Herald added a subscriber: aheejin.

This is the follow up on D88493  where we 
flipped the default for llc on AIX.
We would also want to flip the default for Clang as well.


https://reviews.llvm.org/D88737

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/function-sections.c
  lld/COFF/LTO.cpp
  lld/ELF/LTO.cpp
  lld/wasm/LTO.cpp

Index: lld/wasm/LTO.cpp
===
--- lld/wasm/LTO.cpp
+++ lld/wasm/LTO.cpp
@@ -45,6 +45,7 @@
 
   // Always emit a section per function/data with LTO.
   c.Options.FunctionSections = true;
+  c.Options.HasExplicitDataSections = true;
   c.Options.DataSections = true;
 
   c.DisableVerify = config->disableVerify;
Index: lld/ELF/LTO.cpp
===
--- lld/ELF/LTO.cpp
+++ lld/ELF/LTO.cpp
@@ -86,6 +86,7 @@
 
   // Always emit a section per function/datum with LTO.
   c.Options.FunctionSections = true;
+  c.Options.HasExplicitDataSections = true;
   c.Options.DataSections = true;
 
   // Check if basic block sections must be used.
Index: lld/COFF/LTO.cpp
===
--- lld/COFF/LTO.cpp
+++ lld/COFF/LTO.cpp
@@ -66,6 +66,7 @@
   // Always emit a section per function/datum with LTO. LLVM LTO should get most
   // of the benefit of linker GC, but there are still opportunities for ICF.
   c.Options.FunctionSections = true;
+  c.Options.HasExplicitDataSections = true;
   c.Options.DataSections = true;
 
   // Use static reloc model on 32-bit x86 because it usually results in more
Index: clang/test/Driver/function-sections.c
===
--- clang/test/Driver/function-sections.c
+++ clang/test/Driver/function-sections.c
@@ -3,13 +3,15 @@
 // CHECK-FS: -ffunction-sections
 // CHECK-NOFS-NOT: -ffunction-sections
 // CHECK-DS: -fdata-sections
-// CHECK-NODS-NOT: -fdata-sections
+// CHECK-NODS: -fno-data-sections
+// CHECK-DS-DEFAULT-NOT: -fdata-sections
+// CHECK-DS-DEFAULT-NOT: -fno-data-sections
 // CHECK-US-NOT: -fno-unique-section-names
 // CHECK-NOUS: -fno-unique-section-names
 
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1   \
 // RUN: -target i386-unknown-linux \
-// RUN:   | FileCheck --check-prefix=CHECK-NOFS --check-prefix=CHECK-NODS %s
+// RUN:   | FileCheck --check-prefix=CHECK-NOFS --check-prefix=CHECK-DS-DEFAULT %s
 
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1   \
 // RUN: -target i386-unknown-linux \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -991,6 +991,13 @@
   Args.hasArg(OPT_ffunction_sections) ||
   (Opts.BBSections != "none" && Opts.BBSections != "labels");
 
+  if (Args.getLastArg(OPT_fdata_sections) ||
+  Args.getLastArg(OPT_fno_data_sections)) {
+Opts.HasExplicitDataSections = true;
+Opts.DataSections =
+Args.hasFlag(OPT_fdata_sections, OPT_fno_data_sections, false);
+  }
+
   Opts.DataSections = Args.hasArg(OPT_fdata_sections);
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4906,8 +4906,13 @@
 }
   }
 
-  if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,
-   UseSeparateSections)) {
+  if (Arg *A = Args.getLastArg(options::OPT_fdata_sections,
+   options::OPT_fno_data_sections)) {
+if (A->getOption().matches(options::OPT_fdata_sections))
+  CmdArgs.push_back("-fdata-sections");
+else
+  CmdArgs.push_back("-fno-data-sections");
+  } else if (UseSeparateSections) {
 CmdArgs.push_back("-fdata-sections");
   }
 
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -517,6 +517,7 @@
   Options.EnableMachineFunctionSplitter = CodeGenOpts.SplitMachineFunctions;
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = 

[PATCH] D88260: [NFC][FE] Replace TypeSize with StorageUnitSize

[Jason Liu via Phabricator via cfe-commits]

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

LGTM.




Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1749-1750
 
-// Otherwise, allocate just the number of bytes required to store
-// the bitfield.
+  // Otherwise, allocate just the number of bytes required to store
+  // the bitfield.
 } else {

nit: drive-by fix? 
I think we should move this block of comments into the else block.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1768-1770
+// Otherwise, bump the data size up to include the bitfield,
+// including padding up to char alignment, and then remember how
+// bits we didn't use.

nit: same as the above driver-by fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88260

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

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

[Jason Liu via Phabricator via cfe-commits]

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

LGTM.


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

https://reviews.llvm.org/D86790

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

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

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/test/CodeGenCXX/aix-alignment.cpp:16
+// AIX64: %call = call noalias nonnull i8* @_Znam(i64 8)
+B *allocBp() { return new B[0]; }
+

I believe we would also want to add a test for the delete call to verify the 
change we made: 
```
void bar() {
  delete[] allocBp();
}
```


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

https://reviews.llvm.org/D86790

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

[PATCH] D87451: add new clang option -mno-xcoff-visibility

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: llvm/include/llvm/Target/TargetMachine.h:265
+  /// corresponding to -mno-xcoff-visibility.
+  bool getNoXCOFFVisibility() const { return Options.NoXCOFFVisibility; }
+

DiggerLin wrote:
> daltenty wrote:
> > This seems like it needs the corresponding comand-line option for llc added 
> > and an llc test.
> I think it will be in another separate  patch.
I would actually prefer to have that in the same patch, as that would give us a 
full picture. It's not a huge patch even if we combine them. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87451

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

[PATCH] D87451: add new clang option -mno-xcoff-visibility

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/docs/ClangCommandLineReference.rst:2310
 
+.. option:: -mno-xcoff-visibility
+

It's rare to see an option with only the negative form. Could we rename and 
make it a positive form somehow?
Also we would need to move this option to where the m group belongs. 




Comment at: clang/test/CodeGen/aix-no-xcoff-visibility.cpp:1
+// RUN: %clang -target powerpc-unknown-aix -emit-llvm -o - -S  %s  |\
+// RUN:   FileCheck --check-prefix=VISIBILITY-IR %s

I don't think we should call the driver directly in here. 
We should have a separate driver test where we invoke `clang`, and we should 
invoke the front end `clang_cc1` here.



Comment at: clang/test/CodeGen/aix-no-xcoff-visibility.cpp:75
+
+// VISIBILITY-IR:@b = protected global i32 0
+// VISIBILITY-IR:@pramb = hidden global i32 0

Not sure if the IR check is really necessary, since we haven't made any IR 
change here. It's going to be all the same with or without the new -m option. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87451

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

[PATCH] D87029: [AIX] Implement AIX special bitfield related alignment rules

[Jason Liu via Phabricator via cfe-commits]

jasonliu added a comment.

I think it would help the review if we could put the NFC portion(e.g. TypeSize 
-> StorageUnitSize) to a new patch, and give some rationale about the NFC 
change.




Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:624
   /// a byte, but larger units are used if IsMsStruct.
   unsigned char UnfilledBitsInLastUnit;
+  /// LastBitfieldStorageUnitSize - If IsMsStruct, represents the size of the




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

https://reviews.llvm.org/D87029

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

[PATCH] D87914: [AIX][Clang][Driver] Add handling of shared option

[Jason Liu via Phabricator via cfe-commits]

jasonliu accepted this revision.
jasonliu 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/D87914/new/

https://reviews.llvm.org/D87914

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

[PATCH] D87904: [AIX][Clang][Driver] Add handling of nostartfiles option

[Jason Liu via Phabricator via cfe-commits]

jasonliu accepted this revision.
jasonliu 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/D87904/new/

https://reviews.llvm.org/D87904

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:66
 #include "llvm/Frontend/OpenMP/OMPConstants.h"
+#include 
 #include 

Do we need cmath?



Comment at: clang/include/clang/Sema/Sema.h:488
+AlignPackInfo(AlignPackInfo::Mode M, int Num, bool IsAIX)
+: PackAttr(true), AlignMode(M), PackNumber(Num), AIXStack(IsAIX) {}
+

I noticed PackNumber is an unsigned char and we are passing an int type into it.
Could we add an assertion in the constructor to make sure Num would never be 
something that's going to get truncated when it converts to an unsigned char?



Comment at: clang/include/clang/Sema/Sema.h:504
+unsigned getPackNumber() const { return PackNumber; }
+void setPackNumber(int Num) { PackNumber = Num; }
+

I don't see this function gets referenced in this patch. 



Comment at: clang/include/clang/Sema/Sema.h:514
+
+bool operator==(AlignPackInfo Info) const {
+  return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;

I think a normal operator== signature would look like this:
bool operator==(const AlignPackInfo ) const;



Comment at: clang/include/clang/Sema/Sema.h:515
+bool operator==(AlignPackInfo Info) const {
+  return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
+}

This could return true when `PackAttr` in AlignPackInfo are not the same. 
Wouldn't that cause an issue?



Comment at: clang/include/clang/Sema/Sema.h:519
+bool operator!=(AlignPackInfo Info) const {
+  return AlignMode != Info.AlignMode || PackNumber != Info.PackNumber;
+}

You should be able to implement this using "operator==" right?
e.g. return !(*this == Info);




Comment at: clang/include/clang/Sema/Sema.h:524
+/// \brief True if this is a pragma pack attribute,
+// not a pragma align attribute.
+bool PackAttr;





Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:697
 InferAlignment(false), Packed(false), IsUnion(false),
-IsMac68kAlign(false), IsMsStruct(false), UnfilledBitsInLastUnit(0),
-LastBitfieldTypeSize(0), MaxFieldAlignment(CharUnits::Zero()),
-DataSize(0), NonVirtualSize(CharUnits::Zero()),
+IsMac68kAlign(false), IsNaturalAlign(false), IsMsStruct(false),
+UnfilledBitsInLastUnit(0), LastBitfieldTypeSize(0),

This could probably confuse people, as natural alignment is the default on most 
target except AIX.



Comment at: clang/lib/Sema/SemaAttr.cpp:403
   // Warn about modified alignment after #includes.
   if (PrevPackState.CurrentValue != PackStack.CurrentValue) {
 Diag(IncludeLoc, diag::warn_pragma_pack_modified_after_include);

Since we changed the PackStack for it to contain AlignPackInfo instead of 
unsigned. 
This stack no longer only contains Pack information. So we need to rethink 
about how this diagnostic and the one follows should work.
i.e. What's the purpose of these diagnostic? Is it still only for pragma pack 
report? If so, what we are doing here is not correct, since the `CurrentValue` 
could be different, not because of the pragma pack change, but because of the 
pragma align change.
If it's not only for pragma pack any more, but also intend to detect the pragma 
align interaction, then possibly function name and diagnostic needs some 
modification, as they don't match the intent any more.


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

https://reviews.llvm.org/D87702

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

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

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:259
   mutable TypeInfoMap MemoizedTypeInfo;
+  /// /// A cache from types to size and preferred alignment information.
+  mutable TypeInfoMap MemoizedPreferredTypeInfo;

nit: Extra /// at the beginning of the comment could be removed.



Comment at: clang/include/clang/AST/ASTContext.h:2133
+  /// 'arm_sve_vector_bits'. Should only be called if T->isVLST().
+  unsigned getBitwidthForAttributedSveType(const Type *T) const;
+

Maybe you want to move up to a newer base? I don't see how this change belong 
to this patch.



Comment at: clang/lib/AST/ASTContext.cpp:1872
 
-  // This call can invalidate MemoizedTypeInfo[T], so we need a second lookup.
-  TypeInfo TI = getTypeInfoImpl(T);
-  MemoizedTypeInfo[T] = TI;
-  return TI;
+// This call can invalidate MemoizedTypeInfo[T], so we need a second 
lookup.
+TypeInfo TI = getTypeInfoImpl(T, NeedsPreferredAlignment);

nit: This comment needs an update, as it could invalidate 
MemoizedPreferredTypeInfo depending on the flag.



Comment at: clang/lib/AST/ASTContext.cpp:2106
   Width = Target->getDoubleWidth();
-  Align = Target->getDoubleAlign();
+  setAlign(Width, Target->getDoubleAlign());
   break;

I have two concerns here:
1. I feel like we are having duplicate logic between here and 
ASTContext::getPreferredTypeAlign for how to get preferred alignment. 
2.  These logic are too AIX-centric and only modified the places for where 
types are affected by preferred alignment on AIX. What if on some platforms, 
BuiltinType::Float have different preferred alignments? Or Width is not the 
preferred alignment on certain platform for double/long double.



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

Question: 
It looks like getNaturalAlignIndirect and getTypeAlignInChars here are all 
returning ABI alignment.
But according to the comments, we should use a preferred alignment when it's a 
complete object. Isn't this complete object? Or I'm missing something?


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

https://reviews.llvm.org/D86790

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

[PATCH] D87451: add new clang option -mignore-xcoff-visibility

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/docs/ClangCommandLineReference.rst:2310
 
+.. option:: -mignore-xcoff-visibility
+

We should move this option to where all the other -m options resides.



Comment at: clang/docs/ClangCommandLineReference.rst:2312
+
+Ignore all the visibility pragmas and attributes in source code for aix xcoff.
+





Comment at: clang/include/clang/Basic/LangOptions.def:300
 ENUM_LANGOPT(GC, GCMode, 2, NonGC, "Objective-C Garbage Collection mode")
+BENIGN_LANGOPT(IgnoreXCOFFVisibility, 1, 0, "All the visibility pragmas and 
attributes that are specified in the source are ignored in aix.")
 ENUM_LANGOPT(ValueVisibilityMode, Visibility, 3, DefaultVisibility,

nit: AIX XCOFF target



Comment at: clang/include/clang/Driver/Options.td:1939
 def dA : Flag<["-"], "dA">, Alias;
+def mignore_xcoff_visibility : Flag<["-"], "mignore-xcoff-visibility">, 
Group,
+  HelpText<"Ignore all the visibility pragmas and attributes in source code">,

We should move this option to where the m groups belong.



Comment at: clang/lib/AST/Decl.cpp:1487
+ ? CK.forLinkageOnly()
+ : CK);
 }

Question: When I look for `visibility` in CodeGen, I could find some of the 
visibility settings there that does not depend on the query of existing 
visibility settings. For example, some of the vtable generation, which means we 
could still get visibility settings out from IR in some situation?
Does those visibility settings in the CodeGen matters?



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2768
   // The type-visibility mode defaults to the value-visibility mode.
   if (Arg *typeVisOpt = Args.getLastArg(OPT_ftype_visibility)) {
 Opts.setTypeVisibilityMode(parseVisibility(typeVisOpt, Args, Diags));

Question: how should -mignore-xcoff-visiblity interact with -ftype-visibility?



Comment at: clang/test/Driver/ignore-xcoff-visibility.cpp:24
+// RUN: FileCheck -check-prefix=ERROR %s
+
+__attribute__((visibility("hidden"))) void foo_h(int *p) {

I don't feel like this test belongs in the Driver directory. 
There is driver test needed, which is to pass in `-###` and specify 
-mignore-xcoff-visibility to see if it gets passed by the driver.
But the majority of what this test tests seems belong to CodeGen directory.



Comment at: clang/test/Driver/ignore-xcoff-visibility.cpp:62
+#pragma GCC visibility pop
+
+// IGNOREVISIBILITY:@b = global i32 0

Do we also want to test attribute visibility on a type?
For example:
```
class __attribute__((__visibility__("hidden"))) TestClass {
   int foo();
   int bar();
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87451

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

[PATCH] D84534: [AIX] Static init frontend recovery and backend support

[Jason Liu via Phabricator via cfe-commits]

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

Thanks. LGTM.


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

https://reviews.llvm.org/D84534

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

[PATCH] D84534: [AIX] Static init frontend recovery and backend support

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:466
+
+  bool preprocessStructorList(const DataLayout , const Constant *List,
+  SmallVector );

Xiangling_L wrote:
> jasonliu wrote:
> > Xiangling_L wrote:
> > > jasonliu wrote:
> > > > A doxygen comment describe what this function does, and what its return 
> > > > value means, and mention `Structors` is an output argument.
> > > > By looking at what this function does, it seems `buildStructorList` is 
> > > > a better name.
> > > I meant to and should've named this function to 
> > > `preprocessXXStructorList`, let me know if you still prefer 
> > > `buildStructorList`. And if you do, since the underneath of `SmallVector` 
> > > is a variable-sized array, maybe we should try `buildSortedStructorArray`?
> > `preprocess` sounds like we are already having a XXStructorList and now we 
> > try to do something on it. 
> > But right now, we are actually passing in an empty StructorList/Array and 
> > build it from scratch. So I would still prefer the name of `build` in it.
> > I don't mind changing to a more accurate name as you suggested. 
> I think we do have a `XXStructorList` here which is the initializer of 
> `llvm.gloal_ctors`or `llvm.gloal_dtors` array? The usage of this term is  
> consistent with other spots.
My understanding is that before we enter this `preprocessXXStructorList`, we do 
not have a list of XXStructor. We only have a list of `Constant`. This 
functions turns a list of `Constant` to a list of `XXStructor`. 



Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2107
+  if (!isa(List))
+return false;
 

Xiangling_L wrote:
> jasonliu wrote:
> > Return of boolean seems unnecessary. 
> > Callee could check the size of the Structors to decide if they want an 
> > early return or not (or in this particular case, the for loop would just do 
> > nothing and no need for extra condition if you don't mind the call to 
> > getPointerPrefAlignment or assign to 0 to Index)?
> > So we could just return void for this function?
> Yeah, we could do that. But it looks a boolean return value makes the code 
> flow natural. And if any target does want to control the early return in the 
> future, it's flexbile for them to do that. I am wondering is there any big 
> difference between bool and void return value for this function? 
If target need to control early return in the future, they could still do so by 
querying if the output `Structors` is empty or not. 
I just don't want unnecessary returns, as it's one more thing that user of the 
function need to care about when they examine this function. And the user of 
this function would have the same question of why we need to return a boolean 
here.


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

https://reviews.llvm.org/D84534

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

[PATCH] D84534: [AIX] Static init frontend recovery and backend support

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:391
+  /// @param[out] Structors Sorted Structor structs by Priority.
+  /// @return false if List is not an array of '{ i32, void ()*, i8* }' 
structs.
+  bool preprocessXXStructorList(const DataLayout , const Constant *List,

This description is not entirely true. We only see if the array is a 
ConstantArray and returning false. We are not returning false if the array's 
element is not `{ i32, void ()*, i8* }`.



Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:466
+
+  bool preprocessStructorList(const DataLayout , const Constant *List,
+  SmallVector );

Xiangling_L wrote:
> jasonliu wrote:
> > A doxygen comment describe what this function does, and what its return 
> > value means, and mention `Structors` is an output argument.
> > By looking at what this function does, it seems `buildStructorList` is a 
> > better name.
> I meant to and should've named this function to `preprocessXXStructorList`, 
> let me know if you still prefer `buildStructorList`. And if you do, since the 
> underneath of `SmallVector` is a variable-sized array, maybe we should try 
> `buildSortedStructorArray`?
`preprocess` sounds like we are already having a XXStructorList and now we try 
to do something on it. 
But right now, we are actually passing in an empty StructorList/Array and build 
it from scratch. So I would still prefer the name of `build` in it.
I don't mind changing to a more accurate name as you suggested. 



Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2107
+  if (!isa(List))
+return false;
 

Return of boolean seems unnecessary. 
Callee could check the size of the Structors to decide if they want an early 
return or not (or in this particular case, the for loop would just do nothing 
and no need for extra condition if you don't mind the call to 
getPointerPrefAlignment or assign to 0 to Index)?
So we could just return void for this function?



Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2129
   }
-
   // Emit the function pointers in the target-specific order
   llvm::stable_sort(Structors, [](const Structor , const Structor ) {

nit: Missing blank line.



Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1966
+
+Index++;
+  }

nit: Use `++Index` instead. 
https://llvm.org/docs/CodingStandards.html#prefer-preincrement

Or use `Index++` at line 1963 so that we don't need this line.


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

https://reviews.llvm.org/D84534

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

[PATCH] D84534: [AIX] Static init frontend recovery and backend support

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: format:1
+//===-- PPCAsmPrinter.cpp - Print machine instrs to PowerPC assembly 
--===//
+//

Redundant file?



Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:455
   }
 
+  struct Structor {

The new block is not all `Overridable Hooks`, seems better belong in `Coarse 
grained IR lowering routines`.



Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:456
 
+  struct Structor {
+int Priority = 0;

This struct got moved to header, means it's not an implementation detail any 
more. 
We would need doxygen comment on it.



Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:460
+// In most cases, `Key` represents a `ComdatKey`. Except on AIX, a `Key` is
+// used to identify a csect where we should emit `.ref` when needed.
+GlobalValue *Key = nullptr;

I have a slight preference to leave it as ComdatKey, and change the name when 
we actually need to handle `.ref` because without seeing the actual 
implementation for `.ref` we don't know if this is the way we want to pursue. 



Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:466
+
+  bool preprocessStructorList(const DataLayout , const Constant *List,
+  SmallVector );

A doxygen comment describe what this function does, and what its return value 
means, and mention `Structors` is an output argument.
By looking at what this function does, it seems `buildStructorList` is a better 
name.



Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:468
+  SmallVector );
+  /// Targets can override this to change how `llvm.global_ctors` and
+  /// `llvm.global_dtors` lists get handled.

Add a blank line above this.



Comment at: llvm/test/CodeGen/PowerPC/aix-static-init-key-object.ll:6
+
+@llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, 
void ()*, i8* } { i32 65535, void ()* @foo, i8* @v}]
+

Adding this test case would looks like we already decided how to handle .ref in 
clang and llvm. But in fact, we haven't. I would prefer not having this test.


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

https://reviews.llvm.org/D84534

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

[PATCH] D84534: [AIX] Static init frontend recovery and backend support

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1865
+if (isSpecialLLVMGlobalArrayForStaticInit()) {
+  if (GlobalUniqueModuleId.empty()) {
+GlobalUniqueModuleId = getUniqueModuleId();

We will need to move this part of the if statement to the overrided 
`emitXXStructorList` as well. (If we think overriding `emitXXStructorList` and 
calling `emitSpecialLLVMGlobal ` directly is a good idea.)


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

https://reviews.llvm.org/D84534

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

[PATCH] D84534: [AIX] Static init frontend recovery and backend support

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:24
 #include "llvm/Support/Path.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 

We are removing the usage of "getUniqueModuleId" in this file, So I assume this 
include could get removed as well.



Comment at: clang/lib/CodeGen/CodeGenModule.h:1058
+  /// Add an sterm finalizer to its own llvm.global_dtors entry.
+  void AddCXXStermFinalizerToGlobalDtor(llvm::Function *StermFinalizer,
+int Priority) {

This wrapper seems redundant. Calling ` AddGlobalDtor(StermFinalizer, 
Priority);` directly from the callee side already convey what we are trying to 
achieve here. 



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4602
+llvm::report_fatal_error(
+"prioritized __sinit and __sterm functions are not yet supported");
+  else if (isTemplateInstantiation(D.getTemplateSpecializationKind()) ||

Could we trigger this error? If so, could we have a test for it? 



Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:2119
-unsigned Priority, const MCSymbol *KeySym) const {
-  report_fatal_error("XCOFF dtor section not yet implemented.");
-}

I think it's still useful to keep these functions around to prevent 
accidentally calling to these functions on AIX. We could rephrase error message 
to "static constructor/destructor section is not needed on AIX."



Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1874
+  }
+  emitSpecialLLVMGlobal();
+  continue;

Have some suggestion on structure backend code change:

1. Remove `isSpecialLLVMGlobalArrayForStaticInit` and 
`isSpecialLLVMGlobalArrayToSkip`, and call `emitSpecialLLVMGlobal` directly 
instead. This would handle all the `llvm.` variable for us. We would need do 
early return for names start with `llvm.` in emitGlobalVariable as well, since 
they are all handled here.

2. We could override emitXXStructorList because how XCOFF handle the list is 
vastly different than the other target. We could make the common part(i.e. 
processing and sorting) a utility function, say "preprocessStructorList".

3. It's not necessary to use the same name `emitXXStructor` if the interface is 
different. It might not provide much help when we kinda know no other target is 
going to use this interface. So we could turn it into a lambda inside of the 
overrided emitXXStructorList, or simply no need for a new function because the 
logic is fairly straightforward.



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

https://reviews.llvm.org/D84534

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

[PATCH] D84534: [AIX] Static init frontend recovery and backend support

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4609
+// their own llvm.global_dtors entry.
+CGM.AddCXXStermFinalizerToGlobalDtor(StermFinalizer, 65535);
+  else

Handling template instantiation seems fairly orthogonal to "moving naming logic 
from frontend to backend". Could we put it in a separate patch (which could be 
a child of this one)? 



Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:460
+  virtual void emitXXStructor(const DataLayout , const int Priority,
+  const unsigned index, Constant *CV, bool isCtor) 
{
+llvm_unreachable("Emit CXXStructor with priority is target-specific.");

Remove `const` from value type (e.g. Priority and index), as there is no real 
need for it.



Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2165
+  emitXXStructor(DL, S.Priority, index, S.Func, isCtor);
+  ++index;
+  continue;

Maybe a naive quesiton, in what situation would we have name collision and need 
`index` as suffix to differentiate?
Could we just report_fatal_error in those situation?



Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1848
+// beforing emitting them.
+if (isSpecialLLVMGlobalArrayForStaticInit()) {
+  if (GlobalUniqueModuleId.empty()) {

This would have conflict with D84363. You might want to rebase it later. 



Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1935
+  Function *Func = cast(CV);
+  Func->setLinkage(GlobalValue::ExternalLinkage);
+  Func->setName((isCtor ? llvm::Twine("__sinit") : llvm::Twine("__sterm")) +

Changing Function name and linkage underneath looks a bit scary. People would 
have a hard time to map IR to final assembly that gets produced. Have you 
thought about creating an alias (with the correct linkage and name) to the 
original function instead?


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

https://reviews.llvm.org/D84534



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

[PATCH] D84356: [AIX] remove -u from the clang when invoke aix as assembler

[Jason Liu via Phabricator via cfe-commits]

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

Thanks. LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84356



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

[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] D83974: [AIX] report_fatal_error on `-fregister_global_dtors_with_atexit` for static init

[Jason Liu via Phabricator via cfe-commits]

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

LGTM with minor nit.




Comment at: 
clang/test/CodeGenCXX/aix-sinit-register-global-dtors-with-atexit.cpp:10
+struct T{
+T();
+~T();

nit: fix clang-format warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83974



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

[PATCH] D82476: [Clang][Driver] Recognize the AIX OBJECT_MODE environment setting

[Jason Liu via Phabricator via cfe-commits]

jasonliu accepted this revision.
jasonliu 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/D82476/new/

https://reviews.llvm.org/D82476



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

[PATCH] D81270: [XCOFF][AIX] Use 'L..' instead of '.L' for getPrivateGlobalPrefix in DataLayout

[Jason Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG572dde55eebb: [XCOFF][AIX] Use L.. instead of 
.L for getPrivateGlobalPrefix in DataLayout (authored by jasonliu).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81270

Files:
  clang/lib/Basic/Targets/PPC.h
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DataLayout.h
  llvm/lib/IR/DataLayout.cpp
  llvm/test/CodeGen/PowerPC/aix-lower-constant-pool-index.ll
  llvm/test/CodeGen/PowerPC/aix-lower-jump-table.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll
  llvm/unittests/IR/ManglerTest.cpp

Index: llvm/unittests/IR/ManglerTest.cpp
===
--- llvm/unittests/IR/ManglerTest.cpp
+++ llvm/unittests/IR/ManglerTest.cpp
@@ -136,4 +136,24 @@
 "?vectorcall");
 }
 
+TEST(ManglerTest, XCOFF) {
+  LLVMContext Ctx;
+  DataLayout DL("m:a"); // XCOFF/AIX
+  Module Mod("test", Ctx);
+  Mod.setDataLayout(DL);
+  Mangler Mang;
+  EXPECT_EQ(mangleStr("foo", Mang, DL), "foo");
+  EXPECT_EQ(mangleStr("\01foo", Mang, DL), "foo");
+  EXPECT_EQ(mangleStr("?foo", Mang, DL), "?foo");
+  EXPECT_EQ(mangleFunc("foo", llvm::GlobalValue::ExternalLinkage,
+   llvm::CallingConv::C, Mod, Mang),
+"foo");
+  EXPECT_EQ(mangleFunc("?foo", llvm::GlobalValue::ExternalLinkage,
+   llvm::CallingConv::C, Mod, Mang),
+"?foo");
+  EXPECT_EQ(mangleFunc("foo", llvm::GlobalValue::PrivateLinkage,
+   llvm::CallingConv::C, Mod, Mang),
+"L..foo");
+}
+
 } // end anonymous namespace
Index: llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll
===
--- llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll
+++ llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll
@@ -27,20 +27,20 @@
 
 ; CHECK:   .csect .rodata.str2.2[RO],2
 ; CHECK-NEXT:   .align  1
-; CHECK-NEXT: .Lmagic16:
+; CHECK-NEXT: L..magic16:
 ; CHECK-NEXT:   .vbyte	2, 264 # 0x108
 ; CHECK-NEXT:   .vbyte	2, 272 # 0x110
 ; CHECK-NEXT:   .vbyte	2, 213 # 0xd5
 ; CHECK-NEXT:   .vbyte	2, 0   # 0x0
 ; CHECK-NEXT:   .csect .rodata.str4.4[RO],2
 ; CHECK-NEXT:   .align  2
-; CHECK-NEXT: .Lmagic32:
+; CHECK-NEXT: L..magic32:
 ; CHECK-NEXT:   .vbyte	4, 464 # 0x1d0
 ; CHECK-NEXT:   .vbyte	4, 472 # 0x1d8
 ; CHECK-NEXT:   .vbyte	4, 413 # 0x19d
 ; CHECK-NEXT:   .vbyte	4, 0   # 0x0
 ; CHECK-NEXT:   .csect .rodata.str1.1[RO],2
-; CHECK-NEXT: .LstrA:
+; CHECK-NEXT: L..strA:
 ; CHECK-NEXT: .byte   104
 ; CHECK-NEXT: .byte   101
 ; CHECK-NEXT: .byte   108
@@ -55,7 +55,7 @@
 ; CHECK-NEXT: .byte   33
 ; CHECK-NEXT: .byte   10
 ; CHECK-NEXT: .byte   0
-; CHECK-NEXT: .L.str:
+; CHECK-NEXT: L...str:
 ; CHECK-NEXT: .byte   97
 ; CHECK-NEXT: .byte   98
 ; CHECK-NEXT: .byte   99
Index: llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll
===
--- llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll
+++ llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll
@@ -25,7 +25,7 @@
 ;CHECK: .csect .rodata[RO],4
 
 ;CHECK-NEXT: .align  4
-;CHECK-NEXT: .L__const.main.cnst32:
+;CHECK-NEXT: L..__const.main.cnst32:
 ;CHECK32-NEXT: .vbyte	4, 1073741824
 ;CHECK32-NEXT: .vbyte	4, 50
 ;CHECK64-NEXT: .vbyte	8, 4611686018427387954
@@ -38,7 +38,7 @@
 ;CHECK-NEXT:   .space  4
 
 ;CHECK-NEXT:   .align  3
-;CHECK-NEXT: .L__const.main.cnst16:
+;CHECK-NEXT: L..__const.main.cnst16:
 ;CHECK32-NEXT: .vbyte	4, 1073741824
 ;CHECK32-NEXT: .vbyte	4, 22
 ;CHECK64-NEXT: .vbyte	8, 4611686018427387926
@@ -46,12 +46,12 @@
 ;CHECK-NEXT:   .space  4
 
 ;CHECK-NEXT: .align  3
-;CHECK-NEXT: .L__const.main.cnst8:
+;CHECK-NEXT: L..__const.main.cnst8:
 ;CHECK-NEXT: .vbyte	4, 1073741832  # 0x4008
 ;CHECK-NEXT: .vbyte	4, 0   # 0x0
 
 ;CHECK-NEXT: .align  3
-;CHECK-NEXT: .L__const.main.cnst4:
+;CHECK-NEXT: L..__const.main.cnst4:
 ;CHECK-NEXT: .vbyte	2, 16392   # 0x4008
 ;CHECK-NEXT: .byte   0   # 0x0
 ;CHECK-NEXT: .space  1
Index: llvm/test/CodeGen/PowerPC/aix-lower-jump-table.ll
===
--- llvm/test/CodeGen/PowerPC/aix-lower-jump-table.ll
+++ llvm/test/CodeGen/PowerPC/aix-lower-jump-table.ll
@@ -98,11 +98,11 @@
 ; 32SMALL-ASM: 	blr
 ; 32SMALL-ASM: 	.csect .rodata[RO],2
 ; 32SMALL-ASM: 	.align  2
-; 32SMALL-ASM: .LJTI0_0:
-; 

[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

[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] D74166: [AIX][Frontend] Static init implementation for AIX considering no priority

[Jason Liu via Phabricator via cfe-commits]

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

LGTM. But I suggest to wait for @hubert.reinterpretcast to have another scan at 
this before landing.


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

https://reviews.llvm.org/D74166



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

[PATCH] D74166: [AIX][Frontend] Static init implementation for AIX considering no priority

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:345
+// rarely.
+Weights = nullptr;
+  } else if (Kind == GuardKind::VariableGuard && !D->isLocalVarDecl()) {

Do we need to change/complicate the interface for this function, just to do a 
call to Builder.CreateCondBr()?
Could we call that function directly from where it's needed?



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:648
 
-  // Include the filename in the symbol name. Including "sub_" matches gcc and
-  // makes sure these symbols appear lexicographically behind the symbols with
-  // priority emitted above.
-  SmallString<128> FileName = llvm::sys::path::filename(getModule().getName());
-  if (FileName.empty())
-FileName = "";
+  // Create our global initialization function.
+  if (UseSinitAndSterm && CXXGlobalInits.empty())

This comment does not apply well on top of this early return for some platform. 
I think you could move it to current line 665?


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

https://reviews.llvm.org/D74166



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

[PATCH] D81972: [NFC] Cleanup of EmitCXXGlobalInitFunc() and EmitCXXGlobalDtorFunc()

[Jason Liu via Phabricator via cfe-commits]

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

LGTM.




Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:596
   }
 
+  // Include the filename in the symbol name. Including "sub_" matches gcc

Xiangling_L wrote:
> jasonliu wrote:
> > jasonliu wrote:
> > > jasonliu wrote:
> > > > I think this patch is missing what @hubert.reinterpretcast mentioned in 
> > > > https://reviews.llvm.org/D74166?id=269900#inline-751064
> > > > which is an early return like this:
> > > > 
> > > > ```
> > > >  if (CXXGlobalInits.empty())
> > > > return;
> > > > ```
> > > Please double check the above early return is desired though. It seems 
> > > even when CXXGlobalInits is empty, `GenerateCXXGlobalInitFunc` is trying 
> > > to do a lot of things with `Fn` passed in. Later, we also called 
> > > `AddGlobalCtor(Fn)`. So a lot of behavior changes here, we want to make 
> > > sure it's really 'NFC'.
> > FYI,
> > ```
> > class test {
> >   public:
> > test();
> > };
> > test t1 __attribute__((init_priority (300)));
> > ```
> > Compile with `clang  -target x86_64-apple-darwin10   -emit-llvm `
> > With this change, I guess we will not generate @_GLOBAL__sub_I_d.cpp(), and 
> > also that function will not get added into @llvm.global_ctors.
> > I'm not sure if we really need @_GLOBAL__sub_I_d.cpp() on that platform in 
> > this case. But this change does not really qualify as NFC. 
> > If we need to do this change, a test case is necessary, and we better ask 
> > people who familiar with the platform to confirm it's a desired change. 
> Thanks, I am gonna remove this part and keep it as a NFC patch.
Sure. Please update D74166 accordingly.


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

https://reviews.llvm.org/D81972



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

[PATCH] D81972: [NFC] Cleanup of EmitCXXGlobalInitFunc() and EmitCXXGlobalDtorFunc()

[Jason Liu via Phabricator via cfe-commits]

jasonliu added a comment.

New diff does not have context available.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81972



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

[PATCH] D81972: [NFC] Cleanup of EmitCXXGlobalInitFunc() and EmitCXXGlobalDtorFunc()

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:596
   }
 
+  // Include the filename in the symbol name. Including "sub_" matches gcc

jasonliu wrote:
> jasonliu wrote:
> > I think this patch is missing what @hubert.reinterpretcast mentioned in 
> > https://reviews.llvm.org/D74166?id=269900#inline-751064
> > which is an early return like this:
> > 
> > ```
> >  if (CXXGlobalInits.empty())
> > return;
> > ```
> Please double check the above early return is desired though. It seems even 
> when CXXGlobalInits is empty, `GenerateCXXGlobalInitFunc` is trying to do a 
> lot of things with `Fn` passed in. Later, we also called `AddGlobalCtor(Fn)`. 
> So a lot of behavior changes here, we want to make sure it's really 'NFC'.
FYI,
```
class test {
  public:
test();
};
test t1 __attribute__((init_priority (300)));
```
Compile with `clang  -target x86_64-apple-darwin10   -emit-llvm `
With this change, I guess we will not generate @_GLOBAL__sub_I_d.cpp(), and 
also that function will not get added into @llvm.global_ctors.
I'm not sure if we really need @_GLOBAL__sub_I_d.cpp() on that platform in this 
case. But this change does not really qualify as NFC. 
If we need to do this change, a test case is necessary, and we better ask 
people who familiar with the platform to confirm it's a desired change. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81972



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

[PATCH] D81972: [NFC] Cleanup of EmitCXXGlobalInitFunc() and EmitCXXGlobalDtorFunc()

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:596
   }
 
+  // Include the filename in the symbol name. Including "sub_" matches gcc

jasonliu wrote:
> I think this patch is missing what @hubert.reinterpretcast mentioned in 
> https://reviews.llvm.org/D74166?id=269900#inline-751064
> which is an early return like this:
> 
> ```
>  if (CXXGlobalInits.empty())
> return;
> ```
Please double check the above early return is desired though. It seems even 
when CXXGlobalInits is empty, `GenerateCXXGlobalInitFunc` is trying to do a lot 
of things with `Fn` passed in. Later, we also called `AddGlobalCtor(Fn)`. So a 
lot of behavior changes here, we want to make sure it's really 'NFC'.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81972



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

[PATCH] D74166: [AIX][Frontend] Static init implementation for AIX considering no priority

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:835
+
+  llvm::CallInst *CI = nullptr; 
+  if (Arg == nullptr) {

nit: trailing whitespace



Comment at: clang/test/CodeGen/static-init.cpp:31
+namespace test4 {
+  struct Test4 { 
+Test4();

nit: trailing whitespace in this line.
struct Test4 { 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74166



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

[PATCH] D74166: [AIX][Frontend] Static init implementation for AIX considering no priority

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:708
+" based on strong external symbols");
+  GlobalUniqueModuleId = GlobalUniqueModuleId.substr(1);
+}

Correct me if I'm wrong...
`GlobalUniqueModuleId` will always get “initialized" in 
`EmitCXXGlobalInitFunc`. And `EmitCXXGlobalInitFunc` will always run before 
`EmitCXXGlobalCleanUpFunc` in `CodeGenModule::Release()`. So in theory, we do 
not need to initialize it again in here. 
If we could not `assert (!GlobalUniqueModuleId.empty())` here, that tells me in 
`EmitCXXGlobalInitFunc` we effectively get an empty string after 
`GlobalUniqueModuleId = GlobalUniqueModuleId.substr(1);`. So something is not 
right?



Comment at: clang/lib/CodeGen/CodeGenModule.h:471
 
-  /// Global destructor functions and arguments that need to run on 
termination.
+  /// Global destructor functions and arguments that need to run on 
termination;
+  /// When UseSinitAndSterm is set, it instead contains sterm finalizer

Word after `;` should not be Capitalize. We should use small case, or change 
this to `.`.
I would prefer changing it to `.`.



Comment at: clang/test/CodeGen/static-init.cpp:142
+
+// CHECK: ; Function Attrs: noinline nounwind
+// CHECK: define internal void @__finalize__ZZN5test41fEvE11staticLocal() #0 {

Is checking this Attrs necessary?



Comment at: clang/test/CodeGen/static-init.cpp:157
+
+// CHECK: define void 
@__sinit8000_clang_1145401da454a7baad10bfe313c46638() #5 {
+// CHECK: entry:

I think we used to have dso_local marked on sinit and sterm function in 
previous diff. Now they are gone. What's the reason for removing them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74166



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

[PATCH] D81972: [NFC] Cleanup of EmitCXXGlobalInitFunc() and EmitCXXGlobalDtorFunc()

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:596
   }
 
+  // Include the filename in the symbol name. Including "sub_" matches gcc

I think this patch is missing what @hubert.reinterpretcast mentioned in 
https://reviews.llvm.org/D74166?id=269900#inline-751064
which is an early return like this:

```
 if (CXXGlobalInits.empty())
return;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81972



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

[PATCH] D74166: [AIX][Frontend] Static init implementation for AIX considering no priority

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/test/CodeGen/static-init.cpp:8
+// RUN:   FileCheck %s
 
 struct test {

Looks like the non-inline comments are easier to get ignored and missed, so I 
will copy paste the non-inlined comment I previously had:
```
-fregister_global_dtors_with_atexit does not seem to work properly in current 
implementation.
We should consider somehow disabling/report_fatal_error it instead of letting 
it generate invalid code on AIX.
```


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

https://reviews.llvm.org/D74166



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

[PATCH] D74166: [AIX][Frontend] Static init implementation for AIX considering no priority

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/CGCXXABI.h:113
+
+  virtual bool isCXXGlobalInitAndDtorFuncInternal() const { return true; }
+

Xiangling_L wrote:
> jasonliu wrote:
> > Do we need this isCXXGlobalInitAndDtorFuncInternal? Looks like it's always 
> > going to return ! useSinitAndSterm() result.
> AFAIK, not all platforms which use sinit and sterm have external linkage 
> sinit/sterm functions. And also since for 7.2 AIX we are considering change 
> sinit/sterm to internal linkage symbols, I seperate this query out.
I would prefer to create the query in the patch when we actually need it. With 
what we have right now, it seems redundant. 



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:650
+  FTy,
+  (UseSinitAndSterm ? "__sinit8000_clang_" : "_GLOBAL__sub_I_") +
+  FuncSuffix,

It looks like one of the UseSinitAndSterm is redundant. We could have something 
like: 
```
UseSinitAndSterm ? llvm::twine("__sinit8000_clang_") + GlobalUniqueModuleId 
: llvm::twine("_GLOBAL__sub_I_") + getTransformedFileName(getModule())
```
which minimize the use of std::string?
If this is too long, then we could separate them into if statement just like 
what we have in EmitCXXGlobalDtorFunc, or use a lambda.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:692
+  if (UseSinitAndSterm) {
+Fn = CreateGlobalInitOrDestructFunction(
+FTy, llvm::Twine("__sterm8000_clang_") + GlobalUniqueModuleId, FI,

Could we assert !GlobalUniqueModuleId.empty() here?
Right now, it solely relies on `EmitCXXGlobalInitFunc` to run before 
`EmitCXXGlobalDtorFunc` to get `GlobalUniqueModuleId` initialized properly. I 
think it makes sense to put an assert here to make sure it stays in that case 
in the future.  



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:699
+
+  if (!getCXXABI().isCXXGlobalInitAndDtorFuncInternal())
+Fn->setLinkage(llvm::Function::ExternalLinkage);

This could go inside of the `if (UseSinitAndSterm)`.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4479
+
+  llvm::Value *NeedsDestruct = CGF.Builder.CreateIsNull(V, "guard.hasSrterm");
+

jasonliu wrote:
> Srterm is not used in this implementation.
> Suggestion:
> guard.hasDtorCalled
"guard.hasDtor" does not reflect what the meaning. Maybe matching with the 
variable name would make sense: "guard.needsDestruct" or just "needsDestruct"?



Comment at: clang/test/CodeGen/static-init.cpp:15
+
+// CHECK: define internal void @__cxx_global_var_init() #0 {
+// CHECK: entry:

#0 could be removed.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D74166



___
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] D74166: [AIX][Frontend] Static init implementation for AIX considering no priority

[Jason Liu via Phabricator via cfe-commits]

jasonliu added a comment.

-fregister_global_dtors_with_atexit does not seem to work properly in current 
implementation. 
We should consider somehow disabling/report_fatal_error it instead of letting 
it generate invalid code on AIX.




Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:594
+   "Cannot produce a unique identifier for this module based on strong 
"
+   "external symbols.");
+GlobalUniqueModuleId = GlobalUniqueModuleId.substr(1);

I think report_fatal_error is more appropriate here? Since we know some case 
will trigger this, and we do not want those case to silently pass in 
non-assertion mode?



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4452
+
+  // Emit __sterm function to unregister __srterm and call __srterm.
+  emitCXXGlobalVarDeclDestructFunc(D, dtorStub, addr);

This comment is confusing.
This is not emitting `__sterm` function, this is emitting 
`__cxx_global_var_destruct` functions. And again, we do not have `__srterm` 
function in this implementation. 



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4468
+  const CGFunctionInfo  = CGM.getTypes().arrangeNullaryFunction();
+  llvm::Function *StermFn = CGM.CreateGlobalInitOrDestructFunction(
+  FTy, FnName.str(), FI, D.getLocation());

nit: this is not sterm function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74166



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

[PATCH] D74166: [AIX][Frontend] Static init implementation for AIX considering no priority

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:305
+  if (llvm::Function *unatexitFn =
+  dyn_cast(unatexit.getCallee()))
+unatexitFn->setDoesNotThrow();

Is there a valid case that unatexit.getCallee() returns a type which could not 
be cast to llvm::Function?
i.e. Could we use cast instead of dyn_cast?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74166



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

[PATCH] D74166: [AIX][Frontend] Static init implementation for AIX considering no priority

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/AST/ItaniumMangle.cpp:5217
+  else
+Mangler.getStream() << D->getName();
+}

If I understand correctly, this function will come in pair with 
`__cxx_global_var_init`.
`__cxx_global_var_init` use number as suffix in the end to avoid duplication 
when there is more than one of those, but we are using the variable name as 
suffix here instead.
Do we want to use number as suffix here to match what `__cxx_global_var_init` 
does? It would help people to recognize the pairs and make them more symmetric. 



Comment at: clang/lib/CodeGen/CGCXXABI.h:113
+
+  virtual bool isCXXGlobalInitAndDtorFuncInternal() const { return true; }
+

Do we need this isCXXGlobalInitAndDtorFuncInternal? Looks like it's always 
going to return ! useSinitAndSterm() result.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:586
 
+  bool UseSinitAndSterm = getCXXABI().useSinitAndSterm();
+  if (UseSinitAndSterm) {

`const` for UseSinitAndSterm variable?



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:638
+  // Create our global initialization function.
+  if (!CXXGlobalInits.empty()) {
+// Include the filename in the symbol name. When not using sinit and sterm

flip this for an early return?



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:691
+  if (UseSinitAndSterm) {
+std::string FuncSuffix = GlobalUniqueModuleId;
+Fn = CreateGlobalInitOrDestructFunction(

We might want to avoid this string copy construction if possible. 



Comment at: clang/lib/CodeGen/CodeGenModule.h:20
 #include "SanitizerMetadata.h"
+#include "clang/AST/Attr.h"
 #include "clang/AST/DeclCXX.h"

Is this Attr.h needed somewhere in this file?



Comment at: clang/lib/CodeGen/CodeGenModule.h:401
+  /// A unique trailing identifier as a part of sinit/sterm function when
+  /// UserSinitAndSterm set as true.
+  std::string GlobalUniqueModuleId;

nit: UserSinitAndSterm -> UseSinitAndSterm?
and we do not have that property here.
Suggestion:
when getCXXABI().useSinitAndSterm() return true.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4446
+
+  // Create __srterm function for the var decl.
+  llvm::Function *dtorStub = CGF.createAtExitStub(D, dtor, addr);

In this implementation we do not seem to have `__srterm` functions. I think we 
should refer to `__dtor` instead.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4479
+
+  llvm::Value *NeedsDestruct = CGF.Builder.CreateIsNull(V, "guard.hasSrterm");
+

Srterm is not used in this implementation.
Suggestion:
guard.hasDtorCalled



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4481
+
+  llvm::BasicBlock *DestructCheckBlock = 
CGF.createBasicBlock("destruct.check");
+  llvm::BasicBlock *EndBlock = CGF.createBasicBlock("destruct.end");

I think we need a better naming for this and make it consistent with the end 
block below.
As it is for now, `destruct.check` is confusing, as we are not checking 
anything here and we are just going to call destructor directly in this block. 



Comment at: clang/test/CodeGen/static-init.cpp:10
   ~test();
 } t;
 

Would it be better to test static init for at least two global variable?
Testing only one global variable does not show the whole picture of AIX static 
init.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74166



___
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] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7f5d91d3ffed: [clang][AIX] Implement ABIInfo and 
TargetCodeGenInfo for AIX (authored by jasonliu).

Changed prior to commit:
  https://reviews.llvm.org/D79035?vs=263864=264922#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79035

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-complex.c
  clang/test/CodeGen/aix-return.c
  clang/test/CodeGen/aix-struct-arg.c
  clang/test/CodeGen/aix-vaargs.c
  clang/test/CodeGen/aix-vector.c
  clang/test/CodeGen/ppc32-and-aix-struct-return.c
  clang/test/CodeGen/ppc32-dwarf.c
  clang/test/CodeGen/ppc32-struct-return.c
  clang/test/CodeGen/ppc64-dwarf.c
  clang/test/Frontend/aix-unsupported.c

Index: clang/test/Frontend/aix-unsupported.c
===
--- /dev/null
+++ clang/test/Frontend/aix-unsupported.c
@@ -0,0 +1,10 @@
+// REQUIRES: powerpc-registered-target
+// RUN: not %clang_cc1 -triple powerpc-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -triple powerpc-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -triple powerpc64-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -triple powerpc64-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// CHECK: unsupported option
Index: clang/test/CodeGen/ppc64-dwarf.c
===
--- clang/test/CodeGen/ppc64-dwarf.c
+++ clang/test/CodeGen/ppc64-dwarf.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,PPC64
 static unsigned char dwarf_reg_size_table[1024];
 
 int test() {
@@ -119,10 +120,10 @@
 // CHECK-NEXT: store i8 16, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 108), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 109), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 110), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
 // CHECK-NEXT: ret i32 1
Index: clang/test/CodeGen/ppc32-dwarf.c
===
--- /dev/null
+++ clang/test/CodeGen/ppc32-dwarf.c
@@ -0,0 +1,126 @@
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,PPC32
+static unsigned char dwarf_reg_size_table[1024];
+
+int test() {
+  __builtin_init_dwarf_reg_size_table(dwarf_reg_size_table);
+
+  return __builtin_dwarf_sp_column();
+}
+
+// CHECK-LABEL: define i32 @test()
+// CHECK: store i8 4, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i32 0, i32 0), align 1
+// CHECK-NEXT:store i8 4, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i32 0, i32 1), align 1
+// CHECK-NEXT: 

[PATCH] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 263864.
jasonliu added a comment.

Remove driver's error.


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

https://reviews.llvm.org/D79035

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-complex.c
  clang/test/CodeGen/aix-return.c
  clang/test/CodeGen/aix-struct-arg.c
  clang/test/CodeGen/aix-vaargs.c
  clang/test/CodeGen/aix-vector.c
  clang/test/CodeGen/ppc32-and-aix-struct-return.c
  clang/test/CodeGen/ppc32-dwarf.c
  clang/test/CodeGen/ppc32-struct-return.c
  clang/test/CodeGen/ppc64-dwarf.c
  clang/test/Frontend/aix-unsupported.c

Index: clang/test/Frontend/aix-unsupported.c
===
--- /dev/null
+++ clang/test/Frontend/aix-unsupported.c
@@ -0,0 +1,10 @@
+// REQUIRES: powerpc-registered-target
+// RUN: not %clang_cc1 -triple powerpc-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -triple powerpc-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -triple powerpc64-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -triple powerpc64-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// CHECK: unsupported option
Index: clang/test/CodeGen/ppc64-dwarf.c
===
--- clang/test/CodeGen/ppc64-dwarf.c
+++ clang/test/CodeGen/ppc64-dwarf.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,PPC64
 static unsigned char dwarf_reg_size_table[1024];
 
 int test() {
@@ -119,10 +120,10 @@
 // CHECK-NEXT: store i8 16, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 108), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 109), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 110), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
 // CHECK-NEXT: ret i32 1
Index: clang/test/CodeGen/ppc32-struct-return.c
===
--- clang/test/CodeGen/ppc32-struct-return.c
+++ /dev/null
@@ -1,88 +0,0 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-freebsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-linux \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -maix-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -msvr4-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-netbsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-openbsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-openbsd 

[PATCH] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

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



Comment at: clang/test/Frontend/aix-unsupported.c:10
+// RUN:   -c %s 2>&1 | FileCheck %s
+// CHECK: unsupported option

Xiangling_L wrote:
> One thing I am not so sure about is that for these two options 
> `-maix-struct-return`,  `-msvr4-struct-return`, do we need to update the 
> `ClangCommandLineReference.rst` since we emit diags `unsupported option 
> '-maix-struct-return' for target 'powerpc-unknown-aix'`
I think the error message is clear enough in itself what user should expect. I 
don't think it's necessary to add to the doc.


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

https://reviews.llvm.org/D79035



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

[PATCH] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 263502.
jasonliu added a comment.

Add error report in clang_cc1 for unsupported option on AIX.


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

https://reviews.llvm.org/D79035

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/aix-complex.c
  clang/test/CodeGen/aix-return.c
  clang/test/CodeGen/aix-struct-arg.c
  clang/test/CodeGen/aix-vaargs.c
  clang/test/CodeGen/aix-vector.c
  clang/test/CodeGen/ppc32-and-aix-struct-return.c
  clang/test/CodeGen/ppc32-dwarf.c
  clang/test/CodeGen/ppc32-struct-return.c
  clang/test/CodeGen/ppc64-dwarf.c
  clang/test/Driver/ppc-unsupported.c
  clang/test/Frontend/aix-unsupported.c

Index: clang/test/Frontend/aix-unsupported.c
===
--- /dev/null
+++ clang/test/Frontend/aix-unsupported.c
@@ -0,0 +1,10 @@
+// REQUIRES: powerpc-registered-target
+// RUN: not %clang_cc1 -triple powerpc-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -triple powerpc-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -triple powerpc64-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -triple powerpc64-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// CHECK: unsupported option
Index: clang/test/Driver/ppc-unsupported.c
===
--- clang/test/Driver/ppc-unsupported.c
+++ clang/test/Driver/ppc-unsupported.c
@@ -7,4 +7,12 @@
 // RUN:   -c %s 2>&1 | FileCheck %s
 // RUN: not %clang -target powerpc64le-unknown-linux -msvr4-struct-return \
 // RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
 // CHECK: unsupported option
Index: clang/test/CodeGen/ppc64-dwarf.c
===
--- clang/test/CodeGen/ppc64-dwarf.c
+++ clang/test/CodeGen/ppc64-dwarf.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,PPC64
 static unsigned char dwarf_reg_size_table[1024];
 
 int test() {
@@ -119,10 +120,10 @@
 // CHECK-NEXT: store i8 16, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 108), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 109), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 110), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
 // CHECK-NEXT: ret i32 1
Index: clang/test/CodeGen/ppc32-struct-return.c
===
--- 

[PATCH] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:1547
 
   // Otherwise, if the type contains an SSE vector type, the alignment is 16.
+  if (Align >= 16 && (isSIMDVectorType(getContext(), Ty) ||

Xiangling_L wrote:
> Also update the comment?
I think the comment is fine. It's still SSE vector type even if we changed the 
query to SIMD. SSE is for x86 and altivec is for Power, but they are both SIMD 
vectors.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:4684
 return false;
   case CodeGenOptions::SRCK_InRegs: // -msvr4-struct-return
 return true;

Xiangling_L wrote:
> I noticed that in patch https://reviews.llvm.org/D76360, Zarko added a check 
> to emit an error for using this option within cc1. But in your patch, this 
> option only emit error when invoked by the driver. Does that mean we are 
> pretty sure this option is doing what we want on AIX?
Are you able to set this CodeGen option when it is disabled in the 
Frontend/CompilerInvocation.cpp?


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

https://reviews.llvm.org/D79035



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

[PATCH] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 262964.
jasonliu marked 5 inline comments as done.
jasonliu added a comment.

Address Xiangling's comment.


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

https://reviews.llvm.org/D79035

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/aix-complex.c
  clang/test/CodeGen/aix-return.c
  clang/test/CodeGen/aix-struct-arg.c
  clang/test/CodeGen/aix-vaargs.c
  clang/test/CodeGen/aix-vector.c
  clang/test/CodeGen/ppc32-and-aix-struct-return.c
  clang/test/CodeGen/ppc32-dwarf.c
  clang/test/CodeGen/ppc32-struct-return.c
  clang/test/CodeGen/ppc64-dwarf.c
  clang/test/Driver/ppc-unsupported.c

Index: clang/test/Driver/ppc-unsupported.c
===
--- clang/test/Driver/ppc-unsupported.c
+++ clang/test/Driver/ppc-unsupported.c
@@ -7,4 +7,12 @@
 // RUN:   -c %s 2>&1 | FileCheck %s
 // RUN: not %clang -target powerpc64le-unknown-linux -msvr4-struct-return \
 // RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
 // CHECK: unsupported option
Index: clang/test/CodeGen/ppc64-dwarf.c
===
--- clang/test/CodeGen/ppc64-dwarf.c
+++ clang/test/CodeGen/ppc64-dwarf.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,PPC64
 static unsigned char dwarf_reg_size_table[1024];
 
 int test() {
@@ -119,10 +120,10 @@
 // CHECK-NEXT: store i8 16, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 108), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 109), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 110), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
 // CHECK-NEXT: ret i32 1
Index: clang/test/CodeGen/ppc32-struct-return.c
===
--- clang/test/CodeGen/ppc32-struct-return.c
+++ /dev/null
@@ -1,88 +0,0 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-freebsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-linux \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -maix-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -msvr4-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-netbsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple 

[PATCH] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 262958.
jasonliu marked 2 inline comments as done.
jasonliu added a comment.

Address Zarko's comment.


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

https://reviews.llvm.org/D79035

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/aix-complex.c
  clang/test/CodeGen/aix-return.c
  clang/test/CodeGen/aix-struct-arg.c
  clang/test/CodeGen/aix-vaargs.c
  clang/test/CodeGen/aix-vector.c
  clang/test/CodeGen/ppc32-and-aix-struct-return.c
  clang/test/CodeGen/ppc32-dwarf.c
  clang/test/CodeGen/ppc32-struct-return.c
  clang/test/CodeGen/ppc64-dwarf.c
  clang/test/Driver/ppc-unsupported.c

Index: clang/test/Driver/ppc-unsupported.c
===
--- clang/test/Driver/ppc-unsupported.c
+++ clang/test/Driver/ppc-unsupported.c
@@ -7,4 +7,12 @@
 // RUN:   -c %s 2>&1 | FileCheck %s
 // RUN: not %clang -target powerpc64le-unknown-linux -msvr4-struct-return \
 // RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
 // CHECK: unsupported option
Index: clang/test/CodeGen/ppc64-dwarf.c
===
--- clang/test/CodeGen/ppc64-dwarf.c
+++ clang/test/CodeGen/ppc64-dwarf.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,PPC64
 static unsigned char dwarf_reg_size_table[1024];
 
 int test() {
@@ -119,10 +120,10 @@
 // CHECK-NEXT: store i8 16, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 108), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 109), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 110), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
 // CHECK-NEXT: ret i32 1
Index: clang/test/CodeGen/ppc32-struct-return.c
===
--- clang/test/CodeGen/ppc32-struct-return.c
+++ /dev/null
@@ -1,88 +0,0 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-freebsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-linux \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -maix-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -msvr4-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-netbsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple 

[PATCH] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 262878.

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

https://reviews.llvm.org/D79035

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/aix-complex.c
  clang/test/CodeGen/aix-return.c
  clang/test/CodeGen/aix-struct-arg.c
  clang/test/CodeGen/aix-vaargs.c
  clang/test/CodeGen/aix-vector.c
  clang/test/CodeGen/ppc32-and-aix-struct-return.c
  clang/test/CodeGen/ppc32-dwarf.c
  clang/test/CodeGen/ppc32-struct-return.c
  clang/test/CodeGen/ppc64-dwarf.c
  clang/test/Driver/ppc-unsupported.c

Index: clang/test/Driver/ppc-unsupported.c
===
--- clang/test/Driver/ppc-unsupported.c
+++ clang/test/Driver/ppc-unsupported.c
@@ -7,4 +7,12 @@
 // RUN:   -c %s 2>&1 | FileCheck %s
 // RUN: not %clang -target powerpc64le-unknown-linux -msvr4-struct-return \
 // RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
 // CHECK: unsupported option
Index: clang/test/CodeGen/ppc64-dwarf.c
===
--- clang/test/CodeGen/ppc64-dwarf.c
+++ clang/test/CodeGen/ppc64-dwarf.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,PPC64
 static unsigned char dwarf_reg_size_table[1024];
 
 int test() {
@@ -119,10 +120,10 @@
 // CHECK-NEXT: store i8 16, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 108), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 109), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 110), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
 // CHECK-NEXT: ret i32 1
Index: clang/test/CodeGen/ppc32-struct-return.c
===
--- clang/test/CodeGen/ppc32-struct-return.c
+++ /dev/null
@@ -1,88 +0,0 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-freebsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-linux \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -maix-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -msvr4-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-netbsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-openbsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4

[PATCH] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 262869.
jasonliu added a comment.

Address comments.
Add in over aligned structure and structure containing vector type handling for 
argument on AIX.


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

https://reviews.llvm.org/D79035

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/aix-complex.c
  clang/test/CodeGen/aix-return.c
  clang/test/CodeGen/aix-struct-arg.c
  clang/test/CodeGen/aix-vaargs.c
  clang/test/CodeGen/aix-vector.c
  clang/test/CodeGen/ppc32-and-aix-struct-return.c
  clang/test/CodeGen/ppc32-dwarf.c
  clang/test/CodeGen/ppc32-struct-return.c
  clang/test/CodeGen/ppc64-dwarf.c
  clang/test/Driver/ppc-unsupported.c

Index: clang/test/Driver/ppc-unsupported.c
===
--- clang/test/Driver/ppc-unsupported.c
+++ clang/test/Driver/ppc-unsupported.c
@@ -7,4 +7,12 @@
 // RUN:   -c %s 2>&1 | FileCheck %s
 // RUN: not %clang -target powerpc64le-unknown-linux -msvr4-struct-return \
 // RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
 // CHECK: unsupported option
Index: clang/test/CodeGen/ppc64-dwarf.c
===
--- clang/test/CodeGen/ppc64-dwarf.c
+++ clang/test/CodeGen/ppc64-dwarf.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,PPC64
 static unsigned char dwarf_reg_size_table[1024];
 
 int test() {
@@ -119,10 +120,10 @@
 // CHECK-NEXT: store i8 16, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 108), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 109), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 110), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
 // CHECK-NEXT: ret i32 1
Index: clang/test/CodeGen/ppc32-struct-return.c
===
--- clang/test/CodeGen/ppc32-struct-return.c
+++ /dev/null
@@ -1,88 +0,0 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-freebsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-linux \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -maix-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -msvr4-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-netbsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s 

[PATCH] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:4368
+
+  return emitVoidPtrVAArg(CGF, VAListAddr, Ty, /*Indirect*/ false, TypeInfo,
+  SlotSize, /*AllowHigher*/ true);

ZarkoCA wrote:
> Is there a reason why Indirect is set to `false` instead of querying for it 
> using `classifyArgumentType(Ty).isIndirect()`?
For how we handle Vaarg in backend, we just expect everything on register(no 
ByVal) even when it's a structure. So the indirect should always be false 
unless there is change in backend implementation.  



Comment at: clang/lib/CodeGen/TargetInfo.cpp:4641
 return false;
   case CodeGenOptions::SRCK_InRegs: // -msvr4-struct-return
 return true;

ZarkoCA wrote:
> Has this option been verified to work correctly on AIX? In 
> https://reviews.llvm.org/D76360 we added a defensive error because we weren't 
> sure whether padding was handled correctly as described in the code. 
Thanks. I think we should disable this option in this patch.



Comment at: clang/test/CodeGen/ppc32-and-aix-struct-return.c:8
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
+// RUN: %clang_cc1 -triple powerpc-unknown-linux \
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX

Xiangling_L wrote:
> Do you mean to check AIX or SVR4?
This file is actually from llvm-project/clang/test/CodeGen/ppc32-struct-return.c
What I did is only adding the check for AIX triple. And apparently, the default 
behavior of powerpc-unknown-linux triple is CHECK-AIX.


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

https://reviews.llvm.org/D79035



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

[PATCH] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 262252.

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

https://reviews.llvm.org/D79035

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/aix-complex.c
  clang/test/CodeGen/aix-return.c
  clang/test/CodeGen/aix-struct-arg.c
  clang/test/CodeGen/aix-vaargs.c
  clang/test/CodeGen/aix-vector.c
  clang/test/CodeGen/ppc32-and-aix-struct-return.c
  clang/test/CodeGen/ppc32-dwarf.c
  clang/test/CodeGen/ppc32-struct-return.c
  clang/test/CodeGen/ppc64-dwarf.c
  clang/test/Driver/ppc-unsupported.c

Index: clang/test/Driver/ppc-unsupported.c
===
--- clang/test/Driver/ppc-unsupported.c
+++ clang/test/Driver/ppc-unsupported.c
@@ -7,4 +7,12 @@
 // RUN:   -c %s 2>&1 | FileCheck %s
 // RUN: not %clang -target powerpc64le-unknown-linux -msvr4-struct-return \
 // RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
 // CHECK: unsupported option
Index: clang/test/CodeGen/ppc64-dwarf.c
===
--- clang/test/CodeGen/ppc64-dwarf.c
+++ clang/test/CodeGen/ppc64-dwarf.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,PPC64
 static unsigned char dwarf_reg_size_table[1024];
 
 int test() {
@@ -119,10 +120,10 @@
 // CHECK-NEXT: store i8 16, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 108), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 109), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 110), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
 // CHECK-NEXT: ret i32 1
Index: clang/test/CodeGen/ppc32-struct-return.c
===
--- clang/test/CodeGen/ppc32-struct-return.c
+++ /dev/null
@@ -1,88 +0,0 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-freebsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-linux \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -maix-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -msvr4-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-netbsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-openbsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4

[PATCH] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 262250.
jasonliu marked 12 inline comments as done.
jasonliu added a comment.

Rebase and address comments.


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

https://reviews.llvm.org/D79035

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/aix-complex.c
  clang/test/CodeGen/aix-return.c
  clang/test/CodeGen/aix-struct-arg.c
  clang/test/CodeGen/aix-vaargs.c
  clang/test/CodeGen/aix-vector.c
  clang/test/CodeGen/ppc32-and-aix-struct-return.c
  clang/test/CodeGen/ppc32-dwarf.c
  clang/test/CodeGen/ppc32-struct-return.c
  clang/test/CodeGen/ppc64-dwarf.c
  clang/test/Driver/ppc-unsupported.c

Index: clang/test/Driver/ppc-unsupported.c
===
--- clang/test/Driver/ppc-unsupported.c
+++ clang/test/Driver/ppc-unsupported.c
@@ -7,4 +7,12 @@
 // RUN:   -c %s 2>&1 | FileCheck %s
 // RUN: not %clang -target powerpc64le-unknown-linux -msvr4-struct-return \
 // RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -maix-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
+// RUN: not %clang -target powerpc64-unknown-aix -msvr4-struct-return \
+// RUN:   -c %s 2>&1 | FileCheck %s
 // CHECK: unsupported option
Index: clang/test/CodeGen/ppc64-dwarf.c
===
--- clang/test/CodeGen/ppc64-dwarf.c
+++ clang/test/CodeGen/ppc64-dwarf.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,PPC64
 static unsigned char dwarf_reg_size_table[1024];
 
 int test() {
@@ -119,10 +120,10 @@
 // CHECK-NEXT: store i8 16, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 108), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 109), align 1
 // CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 110), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
-// CHECK-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 111), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 112), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 113), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 114), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 115), align 1
+// PPC64-NEXT: store i8 8, i8* getelementptr inbounds ([1024 x i8], [1024 x i8]* @dwarf_reg_size_table, i64 0, i64 116), align 1
 // CHECK-NEXT: ret i32 1
Index: clang/test/CodeGen/ppc32-struct-return.c
===
--- clang/test/CodeGen/ppc32-struct-return.c
+++ /dev/null
@@ -1,88 +0,0 @@
-// REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -triple powerpc-unknown-freebsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-linux \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -maix-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
-// RUN: %clang_cc1 -triple powerpc-unknown-linux -msvr4-struct-return \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 -triple powerpc-unknown-netbsd \
-// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-SVR4
-// RUN: %clang_cc1 

[PATCH] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

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



Comment at: clang/test/CodeGen/ppc32-dwarf.c:2
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm %s -o - | FileCheck 
%s
+// RUN: %clang_cc1 -triple powerpc-unknown-unknown -emit-llvm %s -o - | 
FileCheck %s --check-prefixes=CHECK,PPC32
+static unsigned char dwarf_reg_size_table[1024];

Xiangling_L wrote:
> Minor comment:
> Would `PPC32SVR4` compared to `PPC32` make the checking content clearer since 
> PPC32 actually includes AIX target?
Technically, it's PPC32 target except AIX (not restrict to SVR4). So PPC32SVR4 
is not that accurate either. 



Comment at: clang/test/CodeGen/ppc64-dwarf.c:2
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix -emit-llvm %s -o - | 
FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-unknown -emit-llvm %s -o - | 
FileCheck %s --check-prefixes=CHECK,PPC64
 static unsigned char dwarf_reg_size_table[1024];

Xiangling_L wrote:
> Same comment as above.
> s/PPC64/PPC64SVR4?
Same above, and for PPC64 we have Darwin that's actually not SVR4.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79035



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

[PATCH] D79035: [clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX

[Jason Liu via Phabricator via cfe-commits]

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



Comment at: clang/lib/CodeGen/TargetInfo.cpp:4317
+  if (isAggregateTypeForABI(RetTy))
+return getNaturalAlignIndirect(RetTy);
+

Xiangling_L wrote:
> This method uses the ABI alignment of the given aggregate type which I think 
> is not ideal due to our AIX special alignment rule. We need to use preferred 
> alignment in this case.
> Btw also I think it's not necessary for you to rebase your patch on the power 
> alignment patch, I can refresh the testcase when I am dealing with that one.
As it is right now in master, there is no difference between natural alignment 
and preferred alignment for AIX. The tentative direction is to use preferred 
alignment to record the actual alignment on AIX, but it is not finalized yet. I 
would rather leave this part of the work for the patch that's going to 
implement the power alignment rule for AIX.



Comment at: clang/test/CodeGen/aix-vaargs.c:3
+// REQUIRES: asserts
+// RUN: %clang_cc1 -triple powerpc-unknown-aix -emit-llvm -o - %s | FileCheck 
%s --check-prefixes=AIX-COM,AIX-M32
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix -emit-llvm -o - %s | 
FileCheck %s --check-prefixes=AIX-COM,AIX-M64

Xiangling_L wrote:
> Consistent with other testcases to use `AIX32/AIX64`?
I chose AIX-M32/AIX-M64 mainly because the length is the same as AIX-COM so we 
don't need to worry about aligning the space. I would prefer to keep it that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79035



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

[PATCH] D79044: [AIX] Avoid structor alias; die before bad alias codegen

[Jason Liu via Phabricator via cfe-commits]

jasonliu accepted this revision.
jasonliu 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/D79044/new/

https://reviews.llvm.org/D79044



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

[PATCH] D79033: [NFC][clang] Replace raw new/delete with unique_ptr to store ABIInfo in TargetCodeGenInfo

[Jason Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe0c356582d2f: [NFC][clang] Replace raw new/delete with 
unique_ptr to store ABIInfo in… (authored by jasonliu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79033

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h

Index: clang/lib/CodeGen/TargetInfo.h
===
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -43,11 +43,10 @@
 /// codegeneration issues, like target-specific attributes, builtins and so
 /// on.
 class TargetCodeGenInfo {
-  ABIInfo *Info;
+  std::unique_ptr Info = nullptr;
 
 public:
-  // WARNING: Acquires the ownership of ABIInfo.
-  TargetCodeGenInfo(ABIInfo *info = nullptr) : Info(info) {}
+  TargetCodeGenInfo(std::unique_ptr Info) : Info(std::move(Info)) {}
   virtual ~TargetCodeGenInfo();
 
   /// getABIInfo() - Returns ABI info helper for the target.
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -385,7 +385,7 @@
   return Address(PHI, Align);
 }
 
-TargetCodeGenInfo::~TargetCodeGenInfo() { delete Info; }
+TargetCodeGenInfo::~TargetCodeGenInfo() = default;
 
 // If someone can figure out a general rule for this, that would be great.
 // It's probably just doomed to be platform-dependent, though.
@@ -682,7 +682,7 @@
 class DefaultTargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   DefaultTargetCodeGenInfo(CodeGen::CodeGenTypes )
-: TargetCodeGenInfo(new DefaultABIInfo(CGT)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT)) {}
 };
 
 ABIArgInfo DefaultABIInfo::classifyArgumentType(QualType Ty) const {
@@ -772,7 +772,7 @@
 public:
   explicit WebAssemblyTargetCodeGenInfo(CodeGen::CodeGenTypes ,
 WebAssemblyABIInfo::ABIKind K)
-  : TargetCodeGenInfo(new WebAssemblyABIInfo(CGT, K)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT, K)) {}
 
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) const override {
@@ -898,8 +898,8 @@
 
 class PNaClTargetCodeGenInfo : public TargetCodeGenInfo {
  public:
-  PNaClTargetCodeGenInfo(CodeGen::CodeGenTypes )
-: TargetCodeGenInfo(new PNaClABIInfo(CGT)) {}
+   PNaClTargetCodeGenInfo(CodeGen::CodeGenTypes )
+   : TargetCodeGenInfo(std::make_unique(CGT)) {}
 };
 
 void PNaClABIInfo::computeInfo(CGFunctionInfo ) const {
@@ -1140,7 +1140,7 @@
   X86_32TargetCodeGenInfo(CodeGen::CodeGenTypes , bool DarwinVectorABI,
   bool RetSmallStructInRegABI, bool Win32StructABI,
   unsigned NumRegisterParameters, bool SoftFloatABI)
-  : TargetCodeGenInfo(new X86_32ABIInfo(
+  : TargetCodeGenInfo(std::make_unique(
 CGT, DarwinVectorABI, RetSmallStructInRegABI, Win32StructABI,
 NumRegisterParameters, SoftFloatABI)) {}
 
@@ -2343,7 +2343,7 @@
 class X86_64TargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   X86_64TargetCodeGenInfo(CodeGen::CodeGenTypes , X86AVXABILevel AVXLevel)
-  : TargetCodeGenInfo(new X86_64ABIInfo(CGT, AVXLevel)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT, AVXLevel)) {}
 
   const X86_64ABIInfo () const {
 return static_cast(TargetCodeGenInfo::getABIInfo());
@@ -2487,7 +2487,7 @@
 public:
   WinX86_64TargetCodeGenInfo(CodeGen::CodeGenTypes ,
  X86AVXABILevel AVXLevel)
-  : TargetCodeGenInfo(new WinX86_64ABIInfo(CGT, AVXLevel)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT, AVXLevel)) {}
 
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) const override;
@@ -4235,8 +4235,8 @@
 public:
   PPC32TargetCodeGenInfo(CodeGenTypes , bool SoftFloatABI,
  bool RetSmallStructInRegABI)
-  : TargetCodeGenInfo(new PPC32_SVR4_ABIInfo(CGT, SoftFloatABI,
- RetSmallStructInRegABI)) {}
+  : TargetCodeGenInfo(std::make_unique(
+CGT, SoftFloatABI, RetSmallStructInRegABI)) {}
 
   static bool isStructReturnInRegABI(const llvm::Triple ,
  const CodeGenOptions );
@@ -4626,8 +4626,8 @@
   PPC64_SVR4_TargetCodeGenInfo(CodeGenTypes ,
PPC64_SVR4_ABIInfo::ABIKind Kind, bool HasQPX,
bool SoftFloatABI)
-  : TargetCodeGenInfo(new PPC64_SVR4_ABIInfo(CGT, Kind, HasQPX,
- SoftFloatABI)) {}
+  : TargetCodeGenInfo(std::make_unique(
+CGT, Kind, HasQPX, SoftFloatABI)) {}
 
   int getDwarfEHStackPointer(CodeGen::CodeGenModule ) const override {
 // This is recovered from gcc 

[PATCH] D79033: [NFC][clang] Replace raw new/delete with unique_ptr to store ABIInfo in TargetCodeGenInfo

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 260766.

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

https://reviews.llvm.org/D79033

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h

Index: clang/lib/CodeGen/TargetInfo.h
===
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -43,11 +43,10 @@
 /// codegeneration issues, like target-specific attributes, builtins and so
 /// on.
 class TargetCodeGenInfo {
-  ABIInfo *Info;
+  std::unique_ptr Info = nullptr;
 
 public:
-  // WARNING: Acquires the ownership of ABIInfo.
-  TargetCodeGenInfo(ABIInfo *info = nullptr) : Info(info) {}
+  TargetCodeGenInfo(std::unique_ptr Info) : Info(std::move(Info)) {}
   virtual ~TargetCodeGenInfo();
 
   /// getABIInfo() - Returns ABI info helper for the target.
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -385,7 +385,7 @@
   return Address(PHI, Align);
 }
 
-TargetCodeGenInfo::~TargetCodeGenInfo() { delete Info; }
+TargetCodeGenInfo::~TargetCodeGenInfo() = default;
 
 // If someone can figure out a general rule for this, that would be great.
 // It's probably just doomed to be platform-dependent, though.
@@ -682,7 +682,7 @@
 class DefaultTargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   DefaultTargetCodeGenInfo(CodeGen::CodeGenTypes )
-: TargetCodeGenInfo(new DefaultABIInfo(CGT)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT)) {}
 };
 
 ABIArgInfo DefaultABIInfo::classifyArgumentType(QualType Ty) const {
@@ -772,7 +772,7 @@
 public:
   explicit WebAssemblyTargetCodeGenInfo(CodeGen::CodeGenTypes ,
 WebAssemblyABIInfo::ABIKind K)
-  : TargetCodeGenInfo(new WebAssemblyABIInfo(CGT, K)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT, K)) {}
 
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) const override {
@@ -898,8 +898,8 @@
 
 class PNaClTargetCodeGenInfo : public TargetCodeGenInfo {
  public:
-  PNaClTargetCodeGenInfo(CodeGen::CodeGenTypes )
-: TargetCodeGenInfo(new PNaClABIInfo(CGT)) {}
+   PNaClTargetCodeGenInfo(CodeGen::CodeGenTypes )
+   : TargetCodeGenInfo(std::make_unique(CGT)) {}
 };
 
 void PNaClABIInfo::computeInfo(CGFunctionInfo ) const {
@@ -1140,7 +1140,7 @@
   X86_32TargetCodeGenInfo(CodeGen::CodeGenTypes , bool DarwinVectorABI,
   bool RetSmallStructInRegABI, bool Win32StructABI,
   unsigned NumRegisterParameters, bool SoftFloatABI)
-  : TargetCodeGenInfo(new X86_32ABIInfo(
+  : TargetCodeGenInfo(std::make_unique(
 CGT, DarwinVectorABI, RetSmallStructInRegABI, Win32StructABI,
 NumRegisterParameters, SoftFloatABI)) {}
 
@@ -2334,7 +2334,7 @@
 class X86_64TargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   X86_64TargetCodeGenInfo(CodeGen::CodeGenTypes , X86AVXABILevel AVXLevel)
-  : TargetCodeGenInfo(new X86_64ABIInfo(CGT, AVXLevel)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT, AVXLevel)) {}
 
   const X86_64ABIInfo () const {
 return static_cast(TargetCodeGenInfo::getABIInfo());
@@ -2478,7 +2478,7 @@
 public:
   WinX86_64TargetCodeGenInfo(CodeGen::CodeGenTypes ,
  X86AVXABILevel AVXLevel)
-  : TargetCodeGenInfo(new WinX86_64ABIInfo(CGT, AVXLevel)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT, AVXLevel)) {}
 
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) const override;
@@ -4204,8 +4204,8 @@
 public:
   PPC32TargetCodeGenInfo(CodeGenTypes , bool SoftFloatABI,
  bool RetSmallStructInRegABI)
-  : TargetCodeGenInfo(new PPC32_SVR4_ABIInfo(CGT, SoftFloatABI,
- RetSmallStructInRegABI)) {}
+  : TargetCodeGenInfo(std::make_unique(
+CGT, SoftFloatABI, RetSmallStructInRegABI)) {}
 
   static bool isStructReturnInRegABI(const llvm::Triple ,
  const CodeGenOptions );
@@ -4595,8 +4595,8 @@
   PPC64_SVR4_TargetCodeGenInfo(CodeGenTypes ,
PPC64_SVR4_ABIInfo::ABIKind Kind, bool HasQPX,
bool SoftFloatABI)
-  : TargetCodeGenInfo(new PPC64_SVR4_ABIInfo(CGT, Kind, HasQPX,
- SoftFloatABI)) {}
+  : TargetCodeGenInfo(std::make_unique(
+CGT, Kind, HasQPX, SoftFloatABI)) {}
 
   int getDwarfEHStackPointer(CodeGen::CodeGenModule ) const override {
 // This is recovered from gcc output.
@@ -5154,7 +5154,7 @@
 class AArch64TargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   AArch64TargetCodeGenInfo(CodeGenTypes , AArch64ABIInfo::ABIKind Kind)
-  : 

[PATCH] D79033: [NFC][clang] Replace raw new/delete with unique_ptr to store ABIInfo in TargetCodeGenInfo

[Jason Liu via Phabricator via cfe-commits]

jasonliu updated this revision to Diff 260746.
jasonliu added a comment.

Provide default virtual destructor.


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

https://reviews.llvm.org/D79033

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h

Index: clang/lib/CodeGen/TargetInfo.h
===
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -43,12 +43,11 @@
 /// codegeneration issues, like target-specific attributes, builtins and so
 /// on.
 class TargetCodeGenInfo {
-  ABIInfo *Info;
+  std::unique_ptr Info = nullptr;
 
 public:
-  // WARNING: Acquires the ownership of ABIInfo.
-  TargetCodeGenInfo(ABIInfo *info = nullptr) : Info(info) {}
-  virtual ~TargetCodeGenInfo();
+  TargetCodeGenInfo(std::unique_ptr Info) : Info(std::move(Info)) {}
+  virtual ~TargetCodeGenInfo() = default;
 
   /// getABIInfo() - Returns ABI info helper for the target.
   const ABIInfo () const { return *Info; }
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -385,8 +385,6 @@
   return Address(PHI, Align);
 }
 
-TargetCodeGenInfo::~TargetCodeGenInfo() { delete Info; }
-
 // If someone can figure out a general rule for this, that would be great.
 // It's probably just doomed to be platform-dependent, though.
 unsigned TargetCodeGenInfo::getSizeOfUnwindException() const {
@@ -682,7 +680,7 @@
 class DefaultTargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   DefaultTargetCodeGenInfo(CodeGen::CodeGenTypes )
-: TargetCodeGenInfo(new DefaultABIInfo(CGT)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT)) {}
 };
 
 ABIArgInfo DefaultABIInfo::classifyArgumentType(QualType Ty) const {
@@ -772,7 +770,7 @@
 public:
   explicit WebAssemblyTargetCodeGenInfo(CodeGen::CodeGenTypes ,
 WebAssemblyABIInfo::ABIKind K)
-  : TargetCodeGenInfo(new WebAssemblyABIInfo(CGT, K)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT, K)) {}
 
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) const override {
@@ -898,8 +896,8 @@
 
 class PNaClTargetCodeGenInfo : public TargetCodeGenInfo {
  public:
-  PNaClTargetCodeGenInfo(CodeGen::CodeGenTypes )
-: TargetCodeGenInfo(new PNaClABIInfo(CGT)) {}
+   PNaClTargetCodeGenInfo(CodeGen::CodeGenTypes )
+   : TargetCodeGenInfo(std::make_unique(CGT)) {}
 };
 
 void PNaClABIInfo::computeInfo(CGFunctionInfo ) const {
@@ -1140,7 +1138,7 @@
   X86_32TargetCodeGenInfo(CodeGen::CodeGenTypes , bool DarwinVectorABI,
   bool RetSmallStructInRegABI, bool Win32StructABI,
   unsigned NumRegisterParameters, bool SoftFloatABI)
-  : TargetCodeGenInfo(new X86_32ABIInfo(
+  : TargetCodeGenInfo(std::make_unique(
 CGT, DarwinVectorABI, RetSmallStructInRegABI, Win32StructABI,
 NumRegisterParameters, SoftFloatABI)) {}
 
@@ -2334,7 +2332,7 @@
 class X86_64TargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   X86_64TargetCodeGenInfo(CodeGen::CodeGenTypes , X86AVXABILevel AVXLevel)
-  : TargetCodeGenInfo(new X86_64ABIInfo(CGT, AVXLevel)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT, AVXLevel)) {}
 
   const X86_64ABIInfo () const {
 return static_cast(TargetCodeGenInfo::getABIInfo());
@@ -2478,7 +2476,7 @@
 public:
   WinX86_64TargetCodeGenInfo(CodeGen::CodeGenTypes ,
  X86AVXABILevel AVXLevel)
-  : TargetCodeGenInfo(new WinX86_64ABIInfo(CGT, AVXLevel)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT, AVXLevel)) {}
 
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) const override;
@@ -4204,8 +4202,8 @@
 public:
   PPC32TargetCodeGenInfo(CodeGenTypes , bool SoftFloatABI,
  bool RetSmallStructInRegABI)
-  : TargetCodeGenInfo(new PPC32_SVR4_ABIInfo(CGT, SoftFloatABI,
- RetSmallStructInRegABI)) {}
+  : TargetCodeGenInfo(std::make_unique(
+CGT, SoftFloatABI, RetSmallStructInRegABI)) {}
 
   static bool isStructReturnInRegABI(const llvm::Triple ,
  const CodeGenOptions );
@@ -4595,8 +4593,8 @@
   PPC64_SVR4_TargetCodeGenInfo(CodeGenTypes ,
PPC64_SVR4_ABIInfo::ABIKind Kind, bool HasQPX,
bool SoftFloatABI)
-  : TargetCodeGenInfo(new PPC64_SVR4_ABIInfo(CGT, Kind, HasQPX,
- SoftFloatABI)) {}
+  : TargetCodeGenInfo(std::make_unique(
+CGT, Kind, HasQPX, SoftFloatABI)) {}
 
   int getDwarfEHStackPointer(CodeGen::CodeGenModule ) const override {
 // This is recovered from gcc output.
@@ -5154,7 +5152,7 @@
 class