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

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4179
+  // This is calculated from the LLVM and GCC tables and verified
+  // against gcc output.  AFAIK all ABIs use the same encoding.
+
----------------
Minor comment about comment style:
Though I noticed that "AFAIK all ABIs use the same encoding." is from original code, could we adjust it to something like "All PPC ABIs use the same encoding."


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4194
+
+  // 64-76 are various 4-byte or 8-byte special-purpose registers:
+  // 64: mq
----------------
s/64-76/64-67?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4317
+  if (isAggregateTypeForABI(RetTy))
+    return getNaturalAlignIndirect(RetTy);
+
----------------
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.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4336
+    if (CGCXXABI::RecordArgABI RAA = getRecordArgABI(Ty, getCXXABI()))
+      return getNaturalAlignIndirect(Ty, RAA == CGCXXABI::RAA_DirectInMemory);
+
----------------
Same comment like above.


================
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
----------------
Consistent with other testcases to use `AIX32/AIX64`?


================
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
----------------
Do you mean to check AIX or SVR4?


================
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];
----------------
Minor comment:
Would `PPC32SVR4` compared to `PPC32` make the checking content clearer since PPC32 actually includes AIX target?


================
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];
----------------
Same comment as above.
s/PPC64/PPC64SVR4?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79035