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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA 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);
----------------
jasonliu wrote:
> This two array gets used to in CC_AIX and here. It would be great if we only have one set of copy. 
I agree, but the array is local to CC_AIX as far as I understand and there is not a good way to access it from here without having to modify CC_AIX.  I think it would be a good to do it at a later point when it's not undergoing so much change already. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7014
 
     if (VA.isRegLoc()) {
       MVT::SimpleValueType SVT = ValVT.getSimpleVT().SimpleTy;
----------------
jasonliu wrote:
> Do we need  `&& !VA.needsCustom() ` here?
Yes, it's better if I add it, thanks.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll:1
+; RUN: llc -O2 -mtriple powerpc-ibm-aix-xcoff -stop-after=machine-cp -mattr=-altivec -verify-machineinstrs < %s | \
+; RUN: FileCheck --check-prefixes=CHECK,32BIT %s
----------------
jasonliu wrote:
> Do we need '-O2' specified here and below?
It makes the IR and assembly much cleaner and easier to read, imo. 


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll:7
+; RUN: FileCheck --check-prefixes=CHECKASM,ASM32PWR4 %s
+
+declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
----------------
jasonliu wrote:
> A general comment about how to test va_arg.
> I think the two test cases we have here are not enough. 
> On top of my head I would probably add in getting double as va_arg argument, uses of va_copy and so on. 
> It's probably worth checking how other targets test va_arg. Grep "va_start" in llvm/test/CodeGen would give you a lot of hit. 
Yes, I can clean up the tests and add more.  Agreed we need more and I can simplify them so it's clear what's being tested. 


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll:15
+
+ at a = local_unnamed_addr global i32 1, align 4
+ at b = local_unnamed_addr global i32 2, align 4
----------------
jasonliu wrote:
> These variables didn't get used anywhere.
Removed, but as I said earlier, I'll also do a rework of the tests. 


================
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:
----------------
jasonliu wrote:
> 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. 
Yes, see reply above, I will rework the tests. 


================
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
----------------
jasonliu wrote:
> a is 1, so we would only call va_arg once, despite we have 9 vaarg argument. Is this the intention of the test?
a = 1 doesn't mean we call va_arg once. This is the IR for the function definition that I got using this c code (modified for this example): 
```
a = 1;
int va_arg1(int a,...) {
...
}
```
The callee doesn't know how many vararg arguments will be called, just does the setup for the variadic parameter (...) in the IR. 


================
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
----------------
jasonliu wrote:
> We only have 2 va_arg arguments here, but we called va_arg 8 times? I think that's undefined behavior. 
Seem my reply above.  That said, there is some confusion with respect to tests and they'll be reworked.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76130