[PATCH] D70126: [PowerPC] Refactor FinishCall

[Sean Fertile via Phabricator via llvm-commits]

sfertile added a comment.

In D70126#1749884 <https://reviews.llvm.org/D70126#1749884>, @cebowleratibm wrote:

> The new logic is much clearer, though it's hard to discern that the semantics remain correct because so much logic was redistributed.  If a series of smaller commits were possible it would be easier to review.


I have to apologize about the size of the patch, its large and more disruptive then I initially hoped for. I attempted to break the patch down into a couple smaller ones but everything I cam up with either had new code that didn't really make sense in the context it was being added into (but did make sense once combined with the other changes) or the intermediate steps were decidedly worse then the starting point. I figured it was less work to review 1 large patch then it was to review several smaller ones in parallel (because they were all needed for context).



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5222
+
+void buildCallOperands(SmallVectorImpl<SDValue> &Ops, CallingConv::ID CallConv,
+                       const SDLoc &dl, bool isTailCall, bool isVarArg,
----------------
jasonliu wrote:
> cebowleratibm wrote:
> > Did you intend buildCallOperands to be static?
> Could this be a static function? 
Yeah. updated.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5290
-      // Restore the caller TOC from the save area into R2.
-      // See PrepareCall() for more information about calls through function
-      // pointers in the 64-bit SVR4 ABI.
----------------
cebowleratibm wrote:
> You've removed PrepareCall and should update the comment.
Good catch.


================
Comment at: llvm/lib/Target/PowerPC/PPCSubtarget.h:352
+  bool usesFunctionDescriptors() const {
+    return isAIXABI() || (is64BitELFABI() && !isELFv2ABI());
+  }
----------------
ZarkoCA wrote:
> I was looking into this, and does the SVR4ABI refer to both ELF v1 and ELF v2? Is that why you can't use isSVR4ABI but instead check that it's not ELFv2? 
SVR4ABI is all the ELF based ABIs: so powerpc32 targeting elf and both the v1 and v2 64-bit powerpc abis targeting elf. The only descriptor based elf abi is 64-bit ELF v1. If I used `isSVR4ABI()` I'd have to add in a `isPPC64()` as well to filter out the 32-bit ELF abi. I'll add a comment to make it easier for the reader to figure out why we are excluding the v2 abi.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70126