[PATCH] D137504: [PowerPC] Implement 64-bit ELFv2 Calling Convention in TableGen (for integers/floats/vectors in registers)

[Amy Kwan via Phabricator via llvm-commits]

amyk added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCCallingConv.cpp:40
+
+  // Handle the shadowing of GPRs for floats and doubles. The number of GPRs
+  // to shadow depends on the size of the argument.
----------------
nemanjai wrote:
> Why does it not suffice for the rest of this function to be something simple like the following:
> ```
> // For single/double precision, shadow a single GPR.
> if (LocVT == MVT::f32 || LocVT == MVT::f64)
>   State.AllocateReg(ELF64ArgGPRs);
> else if (LocVT.is128BitVector() || (LocVT == MVT::f128)) {
>   // For vector and __float128, shadow two even GPRs (skipping
>   // the odd one if it is next in the allocation order).
>   if ((State.AllocateReg(ELF64ArgGPRs) - PPC::X3) % 2 == 0)
>     State.AllocateReg(ELF64ArgGPRs);
>   State.AllocateReg(ELF64ArgGPRs);
>   }
> }
> ```
Thank you for the suggestion Nemanja. I've looked into this and tweaked it slightly. This simplification should suffice.


================
Comment at: llvm/lib/Target/PowerPC/PPCCallingConv.cpp:47-48
+    unsigned GPRIndToShadow = FirstUnallocGPR + 1;
+    for (unsigned RegInd = FirstUnallocGPR; RegInd < GPRIndToShadow; RegInd++)
+      State.AllocateReg(ELF64ArgGPRs[RegInd]);
+  }
----------------
nemanjai wrote:
> Is it possible for this loop to iterate more than once?
I thought it was before, but realized that after I updated the patch after Stefan's comment, this should only run once. Thus, it would make sense to remove this loop. 


================
Comment at: llvm/lib/Target/PowerPC/PPCCallingConv.cpp:51
+  // Handle the shadowing of GPRs for vector arguments.
+  if (LocVT.is128BitVector() || (LocVT == MVT::f128)) {
+    // Shadow one GPR if the first unallocated GPR is an even register number
----------------
nemanjai wrote:
> Please add a comment as to what happens with `MVT::ppcf128`. Does it get broken up into two `MVT::f64`'s before here? Do we just ignore/skip it for now? Should it be an assert until it is implemented if it isn't already implemented?
I have added a comment to state that `MVT::ppcf128` does indeed get broken up to two `MVT::f64`s here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137504