[PATCH] D76360: [PPC][AIX] Emit correct Vaarg for 32BIT-AIX in clang

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA marked 8 inline comments as done.
ZarkoCA added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4175
 namespace {
 /// PPC32_SVR4_ABIInfo - The 32-bit PowerPC ELF (SVR4) ABI information.
 class PPC32_SVR4_ABIInfo : public DefaultABIInfo {
----------------
sfertile wrote:
> This name and comment is misleading, the class is used for both SVR4 and Darwin, and after this patch AIX. We need to fix the name comment to reflect that.
Does this wording of the comment work? 


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4232
                                       QualType Ty) const {
-  if (getTarget().getTriple().isOSDarwin()) {
+  // TODO: Add AIX ABI Info.  Currently we are relying on PPC32_SVR4_ABIInfo to
+  // emit correct VAArg.
----------------
hubert.reinterpretcast wrote:
> No need for two spaces. Add comma after "Currently".
Thanks, I've found it hard to shake the habits my Grade 8 typing teacher taught me. 


================
Comment at: clang/test/CodeGen/aix-vararg.c:4
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -emit-llvm -o - %s | FileCheck %s --check-prefix=32BIT
+#include <stdarg.h>
+
----------------
hubert.reinterpretcast wrote:
> Can we use built-in types and functions in place of a header inclusion?
I'm worried about legal/copyright issues with using contents from AIX system headers to LLVM testcases. But I can do that for sure once I understand what I am allowed to do.   


================
Comment at: clang/test/CodeGen/aix-vararg.c:10
+  va_arg(arg, int);
+  va_end(arg);
+}
----------------
jasonliu wrote:
> As part of a "va_..." family, do we also want to test va_copy? 
Good suggestion, added. 


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

https://reviews.llvm.org/D76360