[PATCH] D74691: [Attributor] Detect possibly unbounded cycles in functions
Stefanos Baziotis via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 26 13:51:22 PST 2020
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
More information about the llvm-commits
mailing list