[PATCH] Teaching loop simplify to preserve LCSSA

[Dinesh Dwivedi]

================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:700-701
@@ -699,4 +699,4 @@
 
-      BI->getSuccessor(0)->removePredecessor(ExitingBlock);
-      BI->getSuccessor(1)->removePredecessor(ExitingBlock);
+      BI->getSuccessor(0)->removePredecessor(ExitingBlock, true);
+      BI->getSuccessor(1)->removePredecessor(ExitingBlock, true);
       ExitingBlock->eraseFromParent();
----------------
Chandler Carruth wrote:
> This seems like a ... very heavy hammer. And yet, it is used pervasively. I think this code should be written differently, but your patch doesn't make it any worse.
> 
> If you do this, you should make the actual LoopSimplify pass preserve LCSSA.
> 
> It would also be good to write some direct test cases for LoopSimplify preserving LCSSA.
It has nothing to do with making LoopSimply pass preserve LCSSA. Sorry for 
confusing title. I have missed this fact that it is a pass. I think 
LoopSimplify need not preserve LCSSA.

What I am trying to solve with this patch is how SimplifyLoop affect LCSSA
when called from UnrollLoop(). I was working on PR18861, crash due to LCSSA
invalidation in LoopUnroll pass and during analysis, I observed that it is 
happening when we merge/ delete some block which results in PHIs with only 
one in-coming path which gets removed by merge/ delete routine. I saw similar 
change by you and tried to fix [http://reviews.llvm.org/D2976] it by reforming
LCSSA in such cases. 

This patch is on same line, given IR in LCSSA form, and we maintaining it through pass,
it was getting invalidated due to SimplifyLoop call at the end of pass and reason was
same, deleting PHI nodes with one in-coming path.

Now, I think we should update SimplifyLoop() to take an argument like 'DontDeleteUselessPHIs'
and selectively keep PHI nodes to preserve LCSSA when needed.

================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:472-473
@@ -471,5 +471,4 @@
     if (OuterL) {
-      ScalarEvolution *SE = PP->getAnalysisIfAvailable<ScalarEvolution>();
-      simplifyLoop(OuterL, DT, LI, PP, /*AliasAnalysis*/ 0, SE);
-      formLCSSARecursively(*OuterL, *DT, SE);
+      simplifyLoop(OuterL, DT, LI, PP, /*AliasAnalysis*/ 0,
+                   PP->getAnalysisIfAvailable<ScalarEvolution>());
     }
----------------
Chandler Carruth wrote:
> Hmm, I'm a touch hesitant to just remove this. When I first changed the loop pass stuff it was... surprisingly noisy due to how much broke.
> 
> If we're going to switch back, I'd like to do it in a separate commit so its easier to revert if things break. Have you tested this change on any large code bases out of curiosity?
I am ok with anything you decide to do with this. If we go with removing these,
I can create/ test/ submit patch for the same.

I tried building gcc with and without this patch. Both build was successful. I will try
building some more large code. It will be great if you can suggest any code base, good
to verify loop optimization.

http://reviews.llvm.org/D2986