[PATCH] D76130: [PPC][AIX] Implement variadic function handling in LowerFormalArguments_AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7076
+    static const MCPhysReg GPR_64[] = {PPC::X3, PPC::X4, PPC::X5, PPC::X6,
+                                       PPC::X7, PPC::X8, PPC::X9, PPC::X10};
+    unsigned const NumGPArgRegs = array_lengthof(GPR_32);
----------------
This two array gets used to in CC_AIX and here. It would be great if we only have one set of copy. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7077
+                                       PPC::X7, PPC::X8, PPC::X9, PPC::X10};
+    unsigned const NumGPArgRegs = array_lengthof(GPR_32);
+
----------------
Could we add an assert to make sure "array_lengthof(GPR_32) == array_lengthof(GPR_64)"
nit: 
unsigned const -> const unsigned


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7087
+      if (!IsPPC64)
+        VReg = MF.addLiveIn(GPR_32[GPRIndex], &PPC::GPRCRegClass);
+      else
----------------
Could we do 

```
const unsigned VReg = IsPPC64 ? MF.addLiveIn(GPR_64[GPRIndex], &PPC::G8RCRegClass)
                              : MF.addLiveIn(GPR_32[GPRIndex], &PPC::GPRCRegClass);
```


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll:27
+; Function Attrs: nounwind
+define i32 @va_arg1(i32 %a, ...) local_unnamed_addr {
+entry:
----------------
These tests used for loops to call va_arg mutliple times. 
Maybe it's just me, but I found the test cases are much harder to read. 
I would suggest maybe just unroll the for loop and generate as much as the va_arg call that you need? 

For the cases below, I suppose you would want to call va_arg 9 times, and 2 times respectfully. Maybe it's not that bad. If the tests gets too long, you could consider split the tests into separate files. 


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll:33
+  call void @llvm.va_start(i8* nonnull %0)
+  %cmp7 = icmp sgt i32 %a, 0
+  br i1 %cmp7, label %for.body.preheader, label %for.end
----------------
a is 1, so we would only call va_arg once, despite we have 9 vaarg argument. Is this the intention of the test?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll:162
+  %add7 = add nsw i32 %add6, %eight
+  %cmp15 = icmp sgt i32 %eight, 0
+  br i1 %cmp15, label %for.body.preheader, label %for.end
----------------
We only have 2 va_arg arguments here, but we called va_arg 8 times? I think that's undefined behavior. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76130