[PATCH] D36736: [PowerPC] Check if the pre-increment preparations have already been made so that they are not done twice

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:65
 
+STATISTIC(PHINodeAlreadyExists, "The PHI Node already exists.");
+
----------------
How about "PHI node already in pre-increment form" (no period at the end).


================
Comment at: lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:193
+                                        const SCEVConstant *BasePtrIncSCEV) {
+  bool FoundPHI = false;
+
----------------
Remove this variable, it is not needed (and see comment below).


================
Comment at: lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:206
+  // Run through the PHIs and see if we have some that looks like a preparation
+  iterator_range< BasicBlock::phi_iterator > PhiIter = BB->phis();
+  for (auto & CurrentPhi : PhiIter) {
----------------
Extra spaces inside < >.


================
Comment at: lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:207
+  iterator_range< BasicBlock::phi_iterator > PhiIter = BB->phis();
+  for (auto & CurrentPhi : PhiIter) {
+    PHINode *PhiNode = dyn_cast<PHINode>(&CurrentPhi);
----------------
Please be consistent in capitalization. PHI is more common in LLVM than Phi, and we use PHI already in this source file. Make this PHIInter and so on.


================
Comment at: lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:235
+          //  PHI that we wanted to create.
+          FoundPHI = true;
+        }
----------------
FoundPHI is not needed. Instead of setting FoundPHI to true here, you can just

  ++PHINodeAlreadyExists;
  return true;


https://reviews.llvm.org/D36736