[PATCH] D30900: [PPC] Eliminate stack frame in non-leaf function based on shrink wrapping

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi,

I haven't looked at the patch proper yet.
Inlined a couple of stylistic comment.

One high level comment, I am not super confortable to change the "status" of the shrink-wrapping pass from a pure analysis to an optimization, i.e., with this patch we actually change the code in the machine function. That might be the right thing to do, but I have to have a closer look to give an informed opinion.

Cheers,
-Quentin



================
Comment at: include/llvm/Target/TargetInstrInfo.h:162
+
+  // If direct return is not supprted on the target, it returns -1.
+  virtual unsigned getCallOpcodeForDirectReturn() const { return (unsigned)-1; }
----------------
Use /// (doxygen style comment)


================
Comment at: include/llvm/Target/TargetInstrInfo.h:162
+
+  // If direct return is not supprted on the target, it returns -1.
+  virtual unsigned getCallOpcodeForDirectReturn() const { return (unsigned)-1; }
----------------
qcolombet wrote:
> Use /// (doxygen style comment)
supported


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:87
+STATISTIC(NumDirectReturn,
+          "Number of method calls applyed direct return optimization");
 
----------------
applied


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:137
   MachineFunction *MachineFunc;
+  /// Call instructions that can be directly return from the callee of
+  /// this method to the caller of this method (by skinpping this method).
----------------
can directly return


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:146
 
+  void FindDirectReturnCandidates(const TargetInstrInfo &TII, RegScavenger *RS);
+
----------------
Move the comment of what this method is doing here for consistency.


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:146
 
+  void FindDirectReturnCandidates(const TargetInstrInfo &TII, RegScavenger *RS);
+
----------------
qcolombet wrote:
> Move the comment of what this method is doing here for consistency.
Lower case for the first letter of the method name.


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:441
 
+/// This function iterates Machine Basic Blocks to find the targets of
+/// the direct return optimization.
----------------
Iterate over the basic block etc.


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:441
 
+/// This function iterates Machine Basic Blocks to find the targets of
+/// the direct return optimization.
----------------
qcolombet wrote:
> Iterate over the basic block etc.
What do you mean by target?
Candidate?


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:443
+/// the direct return optimization.
+/// With the direct return, we allow the callee of this method directly
+/// pass the control to the caller of this method when the callee returns.
----------------
to directly


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:443
+/// the direct return optimization.
+/// With the direct return, we allow the callee of this method directly
+/// pass the control to the caller of this method when the callee returns.
----------------
qcolombet wrote:
> to directly
You mean the current MF as callee, right?


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:463
+    MachineBasicBlock *NextMBB = *(MBB.succ_begin());
+    assert(NextMBB != NULL);
+    if (!NextMBB->isReturnBlock())
----------------
Add a message in the assert, eg. By construction we must have one successor here


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:493
+      // If this BB accesses a CSR, or there is another instructions after call
+      // we cannot optimize this BB
+      if ((CI != NULL && !MI.getDesc().isUnconditionalBranch()) ||
----------------
Period


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:624
+  if (!DirectReturnCandidates.empty()) {
+    // need to confirm that this block is not dominated by the save point
+    const MCInstrDesc &MCID = TII.get(DirectReturnCallOpcode);
----------------
Proper sentence please (Capital letter at the beginning and period at the end).
See http://llvm.org/docs/CodingStandards.html#commenting


https://reviews.llvm.org/D30900