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

[omar ahmed via Phabricator via llvm-commits]

omarahmed added a comment.

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 :)
I will try to submit a diff tmw with this new algorithm with adding a doc :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691