[PATCH] D74691: [Attributor] add some pattern to containsCycle

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

In D74691#1878429 <https://reviews.llvm.org/D74691#1878429>, @jdoerfert wrote:

> In D74691#1878377 <https://reviews.llvm.org/D74691#1878377>, @uenoku wrote:
>
> > In D74691#1878376 <https://reviews.llvm.org/D74691#1878376>, @jdoerfert wrote:
> >
> > > In D74691#1878373 <https://reviews.llvm.org/D74691#1878373>, @uenoku wrote:
> > >
> > > > I'm not sure whether we can say if all the loops given by `LoopInfo` have a max trip count and all the callees are `willreturn`, then we can guarantee the termination of that function.
> > >
> > >
> > > We can not. But if all cycles in the CFG are loops recognized by loop info and they have a max trip count we are good (in this part), I think.
> >
> >
> > How can we confirm all cycles in the CFG are recognized actually by loop info? Is there a simple way?
>
>
> What @omarahmed does here should work (with the fixes I mentioned). As before, we identify all cycles via the depth-first search and the visited set. If a cycles is found we did bail before but now we ask LI & SE instead. If they say we found a proper loop header of a loop with a bounded trip count we can ignore that cycles and continue exploring. Do you think there is a problem with that logic?


This seems fine but I was thinking how we can improve the algorithmic complexity as I believe this is O(N*(N+1)/2) = O(N^2). A cycle is a SCC, so we can use SCCIterator.h (https://llvm.org/doxygen/SCCIterator_8h_source.html), which uses Tarjan, which is O(N+E).
Now since with the current solution we will do a loop analysis on the function anyway, we can get the number of loops in the function. We can then compare if this number is the same as the number of SCCs. If not, we bail.
If yes, we loop through the loops and check if any of them doesn't have a maximum trip count. If yes, we bail. If not, we return `false`.

Edit: Alternatively, we could loop through the SCCs and check if any of them is not a loop but I didn't find an easy way to do that.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2385
+
+        if(LI->isLoopHeader(BB)){
+          Loop *L = LI->getLoopFor(BB);
----------------
The `BB` doesn't have to be a loop header I think as `getLoopFor()` will return you for a BB, quoting the doc:
"the inner most loop that BB lives in". So, any BB in the loop will do it. If the BB not in a loop, it will return you `null` (which you'll have to check).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691