[PATCH] D62164: [PowerPC] Enable MachinePipeliner for P9 with -ppc-enable-pipeliner

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3936
+
+MachineInstr *PPCInstrInfo::findLoopInstr(
+    const MachineBasicBlock &BB, unsigned EndLoopOp,
----------------
FWIW, I find this method a bit strange. Do we need have a MachineLoopInfo available such that we could just check in the preheader of the loop?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3960
+    // Check the predecessors for the LOOP instruction.
+    if (MachineInstr *Loop = findLoopInstr(*PB, EndLoopOp, TargetBB, Visited))
+      return Loop;
----------------
We generally don't directly recurse though the CFG, but use a worklist and a loop. Can you do that here?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:4001
+  // The loop trip count is a run-time value.
+  // We need  to subtract one from the trip count,
+  // and insert branch later to check if we're done with the loop.
----------------
Extra space before 'to'.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:4005
+  // Since BDZ/BDZ8 that we will insert will also decrease the ctr by 1,
+  // so we don't need to generate any thing here
+  Cond.push_back(MachineOperand::CreateImm(0));
----------------
Period at the end of the comment.


================
Comment at: llvm/lib/Target/PowerPC/PPCSubtarget.cpp:192
+bool PPCSubtarget::enableMachinePipeliner() const {
+  return (DarwinDirective == PPC::DIR_PWR9) && EnableMachinePipeliner;
 }
----------------
Given that this is off by default anyway, why is it enabled only for the `P9`?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62164