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

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm marked 6 inline comments as done.
cebowleratibm added a comment.

Minor tweaks and test upates to come.  I'll add an assembly test to validate that we don't need to store to memory to initiailze the two GPRs for f64 in PPC32 mode on newer hardware.



================
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;
----------------
sfertile wrote:
> 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.
A shadow reg is allocated in the same way as any other reg.  What makes it a shadow reg is if there's no associated RegLoc for the register.  This falls out naturally because we only create the RegLoc for vararg calls where we require it be initialized.  The allocated but uninitialized GPRs are already recognized as shadow regs.


================
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()) {
----------------
sfertile wrote:
> 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.
>   
> 
I think it's a good suggestion to move the MemLoc fatal error.  I'll move it to the top of the loop rather than here.

I prefer to use the continue to keep the nesting down rather than use an else.


================
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)
----------------
sfertile wrote:
> 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.
I agree, though I think it can be onerous to manually regex scratch regs.  I'd like to look into using the utils/update_* scripts for test management in the future but I'll finish this test out as per your suggestion.


================
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)
----------------
sfertile wrote:
> 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?
Perhaps a sign that it may be better to write these tests in assembly.  I think it's a good idea to add the coverage you've suggested.


================
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)
----------------
sfertile wrote:
> Ditto on using a filecheck variable for scratch registers.
ack.


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

https://reviews.llvm.org/D71013