[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:186
 
+bool PPCLoopPreIncPrep::alreadyPrepared(Loop *L, Instruction* MemI,
+                                        const SCEV *BasePtrStartSCEV,
----------------
Please add a comment here explaining what this function is doing.


================
Comment at: lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:189
+                                        const SCEVConstant *BasePtrIncSCEV) {
+  bool foundPHI = false;
+
----------------
Local variables start with a capital letter (here and a bunch of places below).


================
Comment at: lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:203
+  iterator_range< BasicBlock::phi_iterator > phi_iter = BB->phis();
+  for ( auto & currentPhi : phi_iter ) {
+    PHINode *phiNode = dyn_cast<PHINode>(&currentPhi);
----------------
Extra spaces inside parens.


================
Comment at: lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:212
+    const SCEV *phiSCEV = SE->getSCEVAtScope(phiNode, L);
+    if (!phiSCEV)
+      continue;
----------------
I don't believe that getSCEVAtScope can return nullptr.


================
Comment at: lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:219
+
+    if (!phiBasePtrSCEV->getStart())
+      continue;
----------------
This can't be null.


================
Comment at: lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:223
+    const SCEVConstant *phiBasePtrIncSCEV =
+    dyn_cast<SCEVConstant>(phiBasePtrSCEV->getStepRecurrence(*SE));
+    if (!phiBasePtrIncSCEV)
----------------
Indentation looks odd here (you might try running clang-format over the patch).


================
Comment at: lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:234
+            phiBasePtrIncSCEV == BasePtrIncSCEV) {
+          // Both the start and the increment are identical.
+          foundPHI = true;
----------------
This phrasing is confusing (it sounds like you're saying that the start and the increment are identical to each other).


================
Comment at: lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:417
 
+    if(alreadyPrepared(L, MemI, BasePtrStartSCEV, BasePtrIncSCEV))
+      continue;
----------------
Space after if.


https://reviews.llvm.org/D36736