[PATCH] D21665: [LoopSimplify] Update LCSSA after separating nested loops.

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:350
@@ +349,3 @@
+  for (BasicBlock *ExitBlock : ExitBlockSet) {
+    for (pred_iterator PI = pred_begin(ExitBlock), PE = pred_end(ExitBlock);
+         PI != PE; ++PI)
----------------
Why not use `predecessors` ?


Actually, why not:

```
if (any_of(predecessors(ExitBlock), [](BasicBlock *BB) {
  return !L->contains(BB);
    }) {
rewriteLoopExitBlock(L, ExitBlock, DT, LI, PreserveLCSSA);
}


================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:368
@@ +367,3 @@
+    L->getExitBlocks(ExitBlocks);
+    SmallSetVector<BasicBlock *, 8> ExitBlockSet(ExitBlocks.begin(),
+                                                 ExitBlocks.end());
----------------
Why do you need to separately create a `SmallSetVector` here?

================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:374
@@ +373,3 @@
+    // preserve LCSSA.
+    for (Instruction &NewI : *NewBB) {
+      PHINode *PN = dyn_cast<PHINode>(&NewI);
----------------
`NewBB` is the header of the new loop, right? (if so, a naming change
would be nice).  Then I'm not sure if scanning `NewBB` for LCSSA
violations is sufficient -- what if we have:

```
loop:
  %iv = phi [ 0, %entry ] [ %iv, (%loop, %left, %right)],  [ %z, %outer ]
  switch(condition), %left, %right, %loop

left:
  %x = def()
  br condition, %outer_l, %loop

outer_l:
  br outer

right:
  %y = def()
  br condition, %outer_r, %loop

outer_r:
  br outer

outer:
  %z = phi(%x, %y)
  br %loop  
```

then the separated structure would be

```
outer_l:
  %iv = phi [ 0, %entry ], [ %z, outer ]
  br loop

loop:
  switch(condition), %left, %right, %loop

left:
  %x = def()
  br condition, %outer_l, %loop

outer_l:
  br outer

right:
  %y = def()
  br condition, %outer, %loop

outer_r:
  br outer

outer:
  %z = phi(%x, %y)  ;; this PHI needs to be edited as well
  br %loop  
```

================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:388
@@ +387,3 @@
+        for (BasicBlock *ExitBB : ExitBlockSet) {
+          if (SSAUpdate.HasValueForBlock(ExitBB))
+            continue;
----------------
When is this true?

================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:393
@@ +392,3 @@
+
+          // We've found the exit block where we need to insert the phi-node.
+          // Create it and register in SSAUpdater.
----------------
s/the exit block/an exit block/
s/the phi-node/an LCSSA phi node/


http://reviews.llvm.org/D21665