[PATCH] D71013: [AIX] Allow vararg calls when all arguments reside in registers.

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6806
-    for (unsigned i = 0; i < StoreSize; i += PtrByteSize)
-      State.AllocateReg(IsPPC64 ? GPR_64 : GPR_32);
     return false;
----------------
We should likely be creating these as shadow regs in the non-vararg case, but I failed to see that in our previous review, sorry. I don't think we need to update that for this patch, but we should before the calling convention work is finished.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7014
 
-    if (VA.isRegLoc())
-      RegsToPass.push_back(std::make_pair(VA.getLocReg(), Arg));
+    if (VA.isRegLoc()) {
+      if (!VA.needsCustom()) {
----------------
Suggestion: I'd prefer we lead with the 2 common cases, then have the special case handling at the end. 

```
if (VA.isRegLoc() && !VA.needsCustom())) {
  RegsToPass.push_back(std::make_pair(VA.getLocReg(), Arg));
} else if(VA.isMemLoc()) {
  /// Impl
} else (VA.isRegLoc()) {
  // Custom handling here.
}
```

I don't fell strongly about this though and think the current form is fine too.
  



================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:627
+
+; 32BIT:      renamable $r3 = LWZtoc @f1, $r2 :: (load 4 from got)
+; 32BIT-NEXT: renamable $f1 = LFS 0, killed renamable $r3 :: (dereferenceable load 4 from @f1)
----------------
When the register is a scratch register like `$r3` is in this instruction we should be using a FileCheck variable instead, to show that the choice of register isn't important.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:634
+; 32BIT-NEXT: renamable $r5 = LWZ 4, %stack.1 :: (load 4 from %stack.1 + 4)
+; 32BIT-NEXT: STFD renamable $f2, 0, %stack.0 :: (store 8 into %stack.0)
+; 32BIT-NEXT: renamable $r6 = LWZ 0, %stack.0 :: (load 4 from %stack.0, align 8)
----------------
IIUC if we were targeting an cpu that has the `mfvsrd` instruction then we wouldn't expect this store to the stack. Do we want to copy this part of the test into a standalone lit test that will ensure we verify there is no store once we enable the VSX extension for AIX?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:642
+
+; 64BIT:      renamable $x3 = LDtoc @f1, $x2 :: (load 8 from got)
+; 64BIT-NEXT: renamable $x4 = LDtoc @d1, $x2 :: (load 8 from got)
----------------
Ditto on using a filecheck variable for scratch registers.


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

https://reviews.llvm.org/D71013