[PATCH] D75013: [LoopTerminology] Rotated Loops

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/docs/LoopTerminology.rst:195-196
+
+This transformation canonicalizes the loop latch to have
+a single successor, which implies that the loop latch
+is also an exiting block. It is done by the `loop-rotate`
----------------
baziotis wrote:
> Meinersbur wrote:
> > baziotis wrote:
> > > Meinersbur wrote:
> > > > [serious] By definition, a latch has a backedge to the header. If the latch had just a single successor, there could not be another edge to outside the loop.
> > > > 
> > > > Using C code to describe the effect is higher level than the IR-level it is actually performed on. What a latch is is not obvious in the C code. LoopRotate also copies the header block, which might be interesting to mention.
> > > > [serious] By definition, a latch has a backedge to the header. If the latch had just a single successor, there could not be another edge to outside the loop.
> > > 
> > > Yes, given that the latch contains the condition (as in a do-while loop), that was not so smart on my part. :P
> > > But honestly, the 2 videos confused me in this part:
> > > - On the one video it says that the latch has a single successor. I assume was a typo.
> > > - On the other, it says that it has a single predecessor. Which, in the example given, I don't think is true. Besides that,
> > > I don't see why having a single predecessor is important. What do I miss?
> > > 
> > > > Using C code to describe the effect is higher level than the IR-level it is actually performed on. What a latch is is not obvious in the C code. LoopRotate also copies the header block, which might be interesting to mention.
> > > 
> > > Yes, I'll change it to IR. How about if I put view-cfg-produced image?
> > Could you point me to where in the videos this statement is made? It is totally possible that we make mistakes as well.
> > 
> > I also would not see why the latch would need to have a single predecessor.
> > 
> > > How about if I put view-cfg-produced image?
> > 
> > In image illustration would be nice.
> > Could you point me to where in the videos this statement is made? It is totally possible that we make mistakes as well.
> > I also would not see why the latch would need to have a single predecessor.
> Of course, no bad intention. I'm not the best to identify the mistake anyway. :)
> So, in the video [[ https://youtu.be/3pRhvQi7Z10?t=287 | Writing Loop Optimizations ]], at around 4:47, there's a slide mentioning the single predecessor.
> Also, in the video [[ https://youtu.be/-JQr9aNagQo?t=330 | Loop Fusion, Loop Distribution and their Place in the Loop Optimization Pipeline ]], at around 5:31, there's a slide mentioning the single successor.
> 
> > In image illustration would be nice.
> Great.
The first presentation talks about a single predecessor and the second about single successor. I think it might refer to [[ https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp#L525 | this ]]:
```
      // Preserve canonical loop form, which means that 'Exit' should have only
      // one predecessor. Note that Exit could be an exit block for multiple
      // nested loops, causing both of the edges to now be critical and need to
      // be split.
```
However, AFAIK it is not directly a requirement for either loop simplified or rotated form, at least it is not checked in `isLoopSimplfyForm` or `isRotatedForm`. That is, I think it is legal (but somewhat strange) in both canonical forms for have a switch in the latch with three cases, one going to the header and two going to (the same) exit block. [[ https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Utils/LoopSimplify.cpp#L604 | Looks like LoopSimplify even reverses this change ]].

Maybe @kbarton can elaborate what was meant.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75013/new/

https://reviews.llvm.org/D75013