[PATCH] D74691: [Attributor] Detect possibly unbounded cycles in functions

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

In D74691#1894376 <https://reviews.llvm.org/D74691#1894376>, @omarahmed wrote:

> In D74691#1894286 <https://reviews.llvm.org/D74691#1894286>, @baziotis wrote:
>
> > In D74691#1893012 <https://reviews.llvm.org/D74691#1893012>, @uenoku wrote:
> >
> > > Could you make sure that you are using a new pass manager? (It means to use `./opt -passes=attributor ..` but not  `./opt -attributor ..`)
> > >  If you are using an old pass manager, you might not get `LoopInfo` or other analysis.
> >
> >
> > Thank you mate, I didn't know I was using the old pass manager.
> >
> > @omarahmed I would advise you to not work anymore on the old algorithm as it's certainly not effective. I'm sorry, but
> >  I barely have time to work on the new algorithm :/ Otherwise, I would try to answer it.
> >
> > So, here's a new algorithm with a couple of debug prints:
> >
> >   static DenseMap<BasicBlock *, unsigned> VisitingTime;
> >   bool containsCycleUtil(BasicBlock *BB) {
> >     dbgs() << "Visiting: " << BB->getName() << "\n";
> >     unsigned VisitingTimeCurrent = VisitingTime[BB];
> >     for (auto *SuccBB : successors(BB)) {
> >       dbgs() << BB->getName() << " -> " << SuccBB->getName() << "\n\n";
> >       unsigned &VisitingTimeSucc = VisitingTime[SuccBB];
> >       if (!VisitingTimeSucc) {
> >         VisitingTimeSucc = VisitingTimeCurrent + 1;
> >         containsCycleUtil(SuccBB);
> >         VisitingTimeSucc = 0;
> >       } else {
> >         dbgs() << "Found cycle: " << VisitingTimeCurrent << ", " << VisitingTimeSucc << "\n";
> >         assert(VisitingTimeCurrent >= VisitingTimeSucc);
> >         unsigned CycleSize = VisitingTimeCurrent - VisitingTimeSucc + 1;
> >         dbgs() << "Cycle size: " << CycleSize << "\n";
> >       }
> >     }
> >     return false;
> >   }
> >  
> >   static bool containsCycle(Attributor &A, Function &F) {
> >     BasicBlock *EntryBB = &F.getEntryBlock();
> >     VisitingTime[EntryBB] = 1;
> >     return containsCycleUtil(EntryBB);
> >   }
> >
> >
> > The main modification compared to the last one is that I track visiting time
> >  instead of going back the recursion. This makes the algorithm so much
> >  cleaner (and reliable, for reasons that are not on topic).
> >  Now, this algorithm, AFAICT, finds **all** cycles, **without false positives**. Plus, it detects
> >  only statically reachable cycles. That's good news, it should be strictly better than the initial.
> >
> > Naively, one could then say "let's apply the same idea as with the SCCs algorithm.
> >  This does not fully work though because check this:
> >
> >   define void @loop_with_if(i1 %c1, i1 %c2) {
> >   entry:
> >     br label %w1
> >  
> >   w1:
> >     br i1 %c1, label %b1, label %exit
> >  
> >   b1:
> >     br label %if
> >  
> >   if:
> >     br i1 %c2, label %t1, label %e1
> >  
> >   t1:
> >     br label %latch
> >  
> >   e1:
> >     br label %latch
> >  
> >   latch:
> >     br label %w1
> >  
> >   exit:
> >     ret void
> >   }
> >
> >
> > If you pay attention, it's a `while` with an `if` inside. If you draw out the CFG, you can
> >  see that there's a cycle: w1 -> b1 -> if -> e1 -> latch -> w1
> >  which the algorithm will (correctly) find. The cycle size is 5 but the loop size is 6.
> >  So, in such loops, even if SCEV can deduce a max trip count (which is possible),
> >  the initial method will fail to realize this is one loop.
> >
> > Which makes a point: There can be a cycle, which is //not// a loop, which still does not
> >  disrupt the maximum trip count guarantee (interestingly, that means that even if
> >  SCCIterator gave us all the SCCs, we would stumble upon this problem).
> >
> > Edit: Well, that was obvious before but not quite in the same context.
> >
> > And there can be many many such real-world cases.
> >  From this point on, the problem I think starts to get quite complicated. It seems we
> >  need to start saying "if all the blocks of the cycle belong to the same loop and
> >  these blocks contain the header and the latch of the loop blah.. blah.." then
> >  this cycle can be skipped.
> >
> > IMHO, we should maybe just live with a not-so-perfect-algorithm, or decide
> >  that it is going to take a good amount of time to get it right.
> >
> > And sorry for the long posts, but this problem turned out to be more complicated
> >  that it initially seemed. I hope I helped a bit.
>
>
> I now understand the problem clearly , Thank you really  , u helped me a lot :)


No problem! If you have any questions, please ask.

> I will try to submit a diff tmw with this new algorithm with adding a doc :)

You may want to submit it initially without the `LoopInfo`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691