[PATCH] D74989: [LoopTerminology] Loop Simplify Form

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis marked an inline comment as done.
baziotis added inline comments.


================
Comment at: llvm/docs/LoopTerminology.rst:152
+* A preheader.
+* A latch which must also be the only backedge.
+* Dedicated exists. That is, no exit block for the loop
----------------
Meinersbur wrote:
> Meinersbur wrote:
> > baziotis wrote:
> > > baziotis wrote:
> > > > nikic wrote:
> > > > > Meinersbur wrote:
> > > > > > baziotis wrote:
> > > > > > > Meinersbur wrote:
> > > > > > > > [serious] There can be a single latch with multiple edges to the header (e.g. a switch)
> > > > > > > Like this?
> > > > > > > ```
> > > > > > > 3:
> > > > > > >   // whatever
> > > > > > >   switch i32 %something, label %whatever [
> > > > > > >     i32 1, label %3
> > > > > > >     i32 2, label %3
> > > > > > >   ]
> > > > > > > ```
> > > > > > > In that case though, isn't the doc in `LoopSimplify.h` wrong?
> > > > > > > "This pass also guarantees that loops will have exactly one backedge."
> > > > > > > Assuming that a latch is a backedge, I figured it should be the only one, what do I miss?
> > > > > > The wording should clarify that whether the simplified form implies a single backedge or a single latch (with potentially multiple edges to the header as in your example). Having a look at the implementation:
> > > > > > ```
> > > > > > bool Loop::isLoopSimplifyForm() const {
> > > > > >   // Normal-form loops have a preheader, a single backedge, and all of their
> > > > > >   // exits have all their predecessors inside the loop.
> > > > > >   return getLoopPreheader() && getLoopLatch() && hasDedicatedExits();
> > > > > > }
> > > > > > 
> > > > > > /// getLoopLatch - If there is a single latch block for this loop, return it.
> > > > > > /// A latch block is a block that contains a branch back to the header.
> > > > > > template <class BlockT, class LoopT>
> > > > > > BlockT *LoopBase<BlockT, LoopT>::getLoopLatch() const {
> > > > > >   assert(!isInvalid() && "Loop not in a valid state!");
> > > > > >   BlockT *Header = getHeader();
> > > > > >   BlockT *Latch = nullptr;
> > > > > >   for (const auto Pred : children<Inverse<BlockT *>>(Header)) {
> > > > > >     if (contains(Pred)) {
> > > > > >       if (Latch)
> > > > > >         return nullptr;
> > > > > >       Latch = Pred;
> > > > > >     }
> > > > > >   }
> > > > > > 
> > > > > >   return Latch;
> > > > > > }
> > > > > > ```
> > > > > > 
> > > > > > And from LoopSimplify:
> > > > > > ```
> > > > > >   BasicBlock *LoopLatch = L->getLoopLatch();
> > > > > >   if (!LoopLatch) {
> > > > > >     ...
> > > > > >     LoopLatch = insertUniqueBackedgeBlock(L, Preheader, DT, LI, MSSAU);
> > > > > >     if (LoopLatch)
> > > > > >       Changed = true;
> > > > > >   }
> > > > > > ```
> > > > > > 
> > > > > > That is, it requires a single backedge (which implies a single latch). Your example is not in simplified form.
> > > > > > 
> > > > > > My wording "There can be a single latch with multiple edges to the header (e.g. a switch)" was not meant to imply that such a case is in simplified form, but that this is possible to happen, i.e. single backedge is not equivalent to single latch.
> > > > > See also D72519...
> > > > First of all, thanks for the explanations!
> > > > 
> > > > > That is, it requires a single backedge (which implies a single latch).
> > > > Yes, exactly!
> > > > > Your example is not in simplified form.
> > > > Yes, my example was just to be sure we're talking for the same thing.
> > > > 
> > > > > My wording "There can be a single latch with multiple edges to the header (e.g. a switch)" was not meant to imply that such a case is in simplified form
> > > > Ah, ok, my bad. That's what I figured since this is a bullet under "For a loop to be in Loop Simplify Form, it must have:".
> > > > 
> > > > > but that this is possible to happen, i.e. single backedge is not equivalent to single latch.
> > > > But not in simplified form right?
> > > > 
> > > > So, honestly, I didn't understand what is wrong with that. :/ It seems that for a loop to be simplified, it must have a latch and a single backedge. Since the latch has a backedge, it must also be the only backedge.
> > > > If you may, please comment how should I change my wording.
> > > > See also D72519...
> > > Thank you, I added me as a subscriber so that if it gets accepted, I should go and change the doc.
> > > 
> > > But not in simplified form right?
> > 
> > If we only talk about loops that are already in simplified form, then "has single backedge" and "has single latch" are tautologies.
> > 
> > > If you may, please comment how should I change my wording.
> > 
> > Change the wording to be about a single backedge (as by the comment in `isLoopSimplifyForm`). You might additionally mention that a single backedge implies a single latch.
> > 
> > The suggestion "A latch which must also be the only backedge." is already wrong because a latch is a basic block (node/vertex in graph terms), not an edge.
> > See also D72519...
> 
> If we change the definition of `getLoopLatch()`, we will have to change the definition of simplified form.
> If we only talk about loops that are already in simplified form, then "has single backedge" and "has single latch" are tautologies.
Huh, ok.

> Change the wording to be about a single backedge (as by the comment in isLoopSimplifyForm). You might additionally mention that a single backedge implies a single latch.
Ok.

> The suggestion "A latch which must also be the only backedge." is already wrong because a latch is a basic block (node/vertex in graph terms), not an edge.
Indeed.


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

https://reviews.llvm.org/D74989