[PATCH] D75013: [LoopTerminology] Rotated Loops

Mon Feb 24 16:18:15 PST 2020

baziotis marked an inline comment as done.
baziotis 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`
----------------
Meinersbur wrote:
> 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.
Thanks for the references! The loop rotation excerpt was not very clear to me. I understood the LoopSimplify one, but seeing the bigger picture I don't understand the reasoning. I'll check tomorrow. Hopefully @kbarton can help.
Btw, reading through these, it seems we should add to the docs what is a deoptimizing edge and what is a critical edge.
It then might be clearer to read the commits: https://github.com/llvm/llvm-project/commit/2f6987ba61cc31c16c64f511e5cbc76b52dc67b3
and
https://github.com/llvm/llvm-project/commit/c8ca49659ac479b5be0349528d42179f40fc553b#diff-7430b177373b789cad7153ebb942f708R11

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

https://reviews.llvm.org/D75013