[PATCH] D32353: [LoopSimplify] Simplify constant conditional branches to unconditional branches

[Anna Thomas via Phabricator via llvm-commits]

anna planned changes to this revision.
anna added a comment.

Based on Eli's comments, this definitely needs more thought.



================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:97
+  // So, we need to update the relevant data structures.
+  // NB: This will never delete any loops completely. It will only delete
+  // blocks within loops.
----------------
efriedma wrote:
> efriedma wrote:
> > This is pretty clearly false... for example, consider:
> > 
> >   OldDest:
> >     br label %OldDest
> > 
> > OldDest is no longer a Loop in the LLVM sense because loops only exist in reachable code.
> > 
> > Or consider the case where OldDest == block; L is not a loop anymore because you deleted the backedge.
> Err, the OldDest == block example doesn't precisely work because the other branch would be an exit.  Better example:
> 
>   block1:
>     br label %block2
> 
>   block2:
>     bi i1 true, label %block2, label %block1
> 
> You're destroying the outer loop by deleting its backedge.
In the first example:
```
OldDest:
  br label %OldDest
```
I was under the impression we are only worried about making a loop become a non-loop by removing back edge (see line 70). From what I understood, we *are removing loops* if a loop that was originally unreachable, i.e. the selfLoop `OldDest` is now *effectively* unreachable. By effectively unreachable, I mean the edge that targets the `OldDest` is deleted (when it was originally unreachable - the branch existed, but it was never taken).


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:97
+  // So, we need to update the relevant data structures.
+  // NB: This will never delete any loops completely. It will only delete
+  // blocks within loops.
----------------
anna wrote:
> efriedma wrote:
> > efriedma wrote:
> > > This is pretty clearly false... for example, consider:
> > > 
> > >   OldDest:
> > >     br label %OldDest
> > > 
> > > OldDest is no longer a Loop in the LLVM sense because loops only exist in reachable code.
> > > 
> > > Or consider the case where OldDest == block; L is not a loop anymore because you deleted the backedge.
> > Err, the OldDest == block example doesn't precisely work because the other branch would be an exit.  Better example:
> > 
> >   block1:
> >     br label %block2
> > 
> >   block2:
> >     bi i1 true, label %block2, label %block1
> > 
> > You're destroying the outer loop by deleting its backedge.
> In the first example:
> ```
> OldDest:
>   br label %OldDest
> ```
> I was under the impression we are only worried about making a loop become a non-loop by removing back edge (see line 70). From what I understood, we *are removing loops* if a loop that was originally unreachable, i.e. the selfLoop `OldDest` is now *effectively* unreachable. By effectively unreachable, I mean the edge that targets the `OldDest` is deleted (when it was originally unreachable - the branch existed, but it was never taken).
The outerloop example is very useful. Thank you! I think even changing the check in line 70, to consider `getLoopFor` for both target blocks would not be enough. Again, it would be making an llvm loop become a non-loop because it is not reachable. Will have to think about this more.


https://reviews.llvm.org/D32353