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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
