[PATCH] D74989: [LoopTerminology] Loop Simplify Form

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis marked 2 inline comments 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
----------------
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.


================
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
----------------
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.



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

https://reviews.llvm.org/D74989