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

[Chuang-Yu Cheng via llvm-commits]

cycheng 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;
----------------
kbarton wrote:
> Variable names should be camel case (i.e., remove the underscores).
> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
Thanks!

================
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())
----------------
kbarton wrote:
> 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. 
Agree!

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:3969
@@ +3968,3 @@
+                                    SelectionDAG& DAG) const {
+  bool TailCallOpt = getTargetMachine().Options.GuaranteedTailCallOpt;
+
----------------
kbarton wrote:
> 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. 
"GuaranteedTailCallOpt" is original code path.

SibCallOpt and TailCallOpt share the following condition checking:
  - isVarArg
  - Same Calling Convention
  - by val parameters checking
  - indirect call checking
  - resideInSameModule checking

Specific for TailCallOpt checking:
  - Calling convention should be "CallingConv::Fast"

Specific for SibCallOpt checking:
  - hasSameArgumentList
  - needStackSlotPassParameters

So even if TallCallOpt is true, there are still some cases that can not be TailCallOptimized.
And if TallCallOpt is true, we don't need to check SibCallOpt condition.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:4671
@@ +4670,3 @@
+
+      assert(isa<GlobalAddressSDNode>(Callee));
+      DEBUG(
----------------
kbarton wrote:
> Please add a message here indicating why this assert fails.
```
      assert(isa<GlobalAddressSDNode>(Callee) &&
             "Callee should be an llvm::Function object.");
```



http://reviews.llvm.org/D16315