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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4368
+
+  return emitVoidPtrVAArg(CGF, VAListAddr, Ty, /*Indirect*/ false, TypeInfo,
+                          SlotSize, /*AllowHigher*/ true);
----------------
Is there a reason why Indirect is set to `false` instead of querying for it using `classifyArgumentType(Ty).isIndirect()`?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4641
     return false;
   case CodeGenOptions::SRCK_InRegs: // -msvr4-struct-return
     return true;
----------------
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. 


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4654
                                                 llvm::Value *Address) const {
-  // This is calculated from the LLVM and GCC tables and verified
-  // against gcc output.  AFAIK all ABIs use the same encoding.
-
-  CodeGen::CGBuilderTy &Builder = CGF.Builder;
-
-  llvm::IntegerType *i8 = CGF.Int8Ty;
-  llvm::Value *Four8 = llvm::ConstantInt::get(i8, 4);
-  llvm::Value *Eight8 = llvm::ConstantInt::get(i8, 8);
-  llvm::Value *Sixteen8 = llvm::ConstantInt::get(i8, 16);
-
-  // 0-31: r0-31, the 4-byte general-purpose registers
-  AssignToArrayRange(Builder, Address, Four8, 0, 31);
-
-  // 32-63: fp0-31, the 8-byte floating-point registers
-  AssignToArrayRange(Builder, Address, Eight8, 32, 63);
-
-  // 64-76 are various 4-byte special-purpose registers:
-  // 64: mq
-  // 65: lr
-  // 66: ctr
-  // 67: ap
-  // 68-75 cr0-7
-  // 76: xer
-  AssignToArrayRange(Builder, Address, Four8, 64, 76);
-
-  // 77-108: v0-31, the 16-byte vector registers
-  AssignToArrayRange(Builder, Address, Sixteen8, 77, 108);
-
-  // 109: vrsave
-  // 110: vscr
-  // 111: spe_acc
-  // 112: spefscr
-  // 113: sfp
-  AssignToArrayRange(Builder, Address, Four8, 109, 113);
-
-  return false;
+  return PPC_initDwarfEHRegSizeTable(CGF, Address, /* Is64Bit*/ false,
+                                     /*IsAIX*/ false);
----------------
Should be changed to `/*Is64BIT*/`? Or you can add spaces to`/*IsAIX*/` so that it's consistent.  


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5195
+  return PPC_initDwarfEHRegSizeTable(CGF, Address, /* Is64Bit*/ true,
+                                     /*IsAIX*/ false);
 }
----------------
Remove extra space from `/* Is64Bit*/`


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10454
+    if (Triple.isOSAIX())
+      return SetCGInfo(new AIXTargetCodeGenInfo(Types, /* Is64Bit */ false));
+
----------------
same as above about removing extra spaces. 


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10465
+    if (Triple.isOSAIX())
+      return SetCGInfo(new AIXTargetCodeGenInfo(Types, /* Is64Bit */ true));
+
----------------
spaces


================
Comment at: clang/test/CodeGen/aix-return.c:3
+// RUN: %clang_cc1 -triple powerpc-unknown-aix \
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefixes=AIX-COM,AIX32
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix \
----------------
I think it would be simpler to just use `AIX` instead of `AIX-COM`.  


================
Comment at: clang/test/CodeGen/aix-struct-arg.c:3
+// RUN: %clang_cc1 -triple powerpc-unknown-aix \
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-AIX
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix \
----------------
Same here, I think it be simpler to use either `CHECK` or `AIX`.  My preference is to use `CHECK` since other targets aren't being tested here but I would leave it up to you. 


================
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
----------------
jasonliu wrote:
> 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.
I agree with Xiangling, it would be better to be consistent with the other testcases.  To get things to line up properly you can use `AIX32/AIX64` and `CHECK` instead of `AIX-COM`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79035