[PATCH] [PR18861] Fix for LoopUnroll pass was breaking LCSSA form while completely removing loop

[Andrew Trick]

On May 9, 2014, at 4:38 PM, Dinesh Dwivedi <dinesh.d at samsung.com> wrote:

> Thanks, I agree that I don't understand complete flow and internals.
> 
> But While debugging for this bug, I observed that there are only 2 places we are messing 
> up with PHI nodes during LoopUnrool pass, one in FoldBlockIntoPredecessor() while folding 
> single entry PHI nodes and other during loop simplify.
> 
> For issue due to loop simplify, Chandler has added formLCSSARecursively() for parent loop
> if current loop is unrolled completely. I saw that if we run formLCSSARecursively() on the 
> root node of the loop nest, it fixes this bug too. But as this was temporary fix as per chandler, 
> I started looking to completely avoid formLCSSARecursively.

Right. But we don’t have to run it on the root, just on the right level of loop nest, which is usually just the parent.

> This patch tries to fix PHIs deleted by FoldBlockIntoPredecessor. I had send another patch 
> to keep PHIs nodes from deletion while simplifying loop.

That could be part of a compile time optimization later, but is an incomplete fix (and not very maintainable).

> I assumed that if loop was in LCSSA and we keep/ re-insert deleted PHI nodes which was 
> there only to keep loop in LCSSA form, that will ensure that loop will be in LCSSA after pass
> and will take care of all corner cases. But I may be wrong.

I don’t have an .ll case right now, but I can easily envision a case where unrolling requires inserting a new phi.

L1
|
L2
|
L3
|
A - L2
|  
B - L1
|
C - L3
|
Exit

If I got it right, after unrolling L3, you need a phi at A->B and B->C.

> I will try to update patch as per your suggestions. 

Thanks. It would be nice to know we have a complete implementation checked in. Then optimize from there.

-Andy

> 
> Again thanks for review and suggestions :).
> 
> http://reviews.llvm.org/D2976
> 
>