[PATCH] D16315: [ppc64] Enable sibling call optimization on ppc64 ELFv1/ELFv2 abi

[Kit Barton via llvm-commits]

kbarton added inline comments.

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:3912
@@ +3911,3 @@
+  const unsigned Num_FPR_Regs = 13;
+  const unsigned Num_VR_Regs = array_lengthof(VR);
+  const unsigned ParamAreaSize = Num_GPR_Regs * PtrByteSize;
----------------
Variable names should be camel case (i.e., remove the underscores).
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:3932
@@ +3931,3 @@
+static bool
+hasSameArgumentList(const Function* CallerFn, ImmutableCallSite *CS) {
+  if (CS->arg_size() != CallerFn->getArgumentList().size())
----------------
I'd recommend const Function *CallerFn, instead of const Function* CallerFn. 
The * modifies the parameter, not the type, so it makes sense to put it closer to the parameter. 

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:3969
@@ +3968,3 @@
+                                    SelectionDAG& DAG) const {
+  bool TailCallOpt = getTargetMachine().Options.GuaranteedTailCallOpt;
+
----------------
The GuaranteedTailCallOpt concerns me. Is this used to guarantee functional correctness in some cases?
There are several checks below that will return false, even if TallCallOpt is true. Are we guaranteed this is always going to be correct?

If it is not for correctness, then why can it not be disabled? It overrides the DisableSCO flag on line 3971, but then is ignored later on line 4008. 

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:4671
@@ +4670,3 @@
+
+      assert(isa<GlobalAddressSDNode>(Callee));
+      DEBUG(
----------------
Please add a message here indicating why this assert fails.

================
Comment at: test/CodeGen/PowerPC/ppc64-sibcall-shrinkwrap.ll:2
@@ +1,3 @@
+; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu --enable-shrink-wrap=false | FileCheck %s -check-prefix=CHECK-SCO-ONLY
+; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu --enable-shrink-wrap=true | FileCheck %s -check-prefix=CHECK-SCO-SHRK
+%"class.clang::NamedDecl" = type { i32 }
----------------
Please add run steps for powerpc64le-unknown-linux-gnu as well.

================
Comment at: test/CodeGen/PowerPC/ppc64-sibcall.ll:2
@@ +1,3 @@
+; RUN: llc < %s -O1 -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu | FileCheck %s -check-prefix=CHECK-SCO
+; RUN: llc < %s -O1 -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr8 | FileCheck %s -check-prefix=CHECK-SCO-HASQPX
+
----------------
Please add run steps for powerpc64le-unknown-linux-gnu as well.


http://reviews.llvm.org/D16315