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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

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

> In D74691#1907179 <https://reviews.llvm.org/D74691#1907179>, @baziotis wrote:
>
> > In D74691#1906930 <https://reviews.llvm.org/D74691#1906930>, @omarahmed wrote:
> >
> > > [Attributor]Detect possibly unbounded cycles in functions
> > >
> > > so the new approach detects cycles by using a recursive function that dfs on the CFG
> >
> >
> > Note that this algorithm detects whether there is //a// cycle in the function (correctl). But now
> >  note that if you find a cycle, you may want to skip it. In that case, it is wrong.
> >  Specifically, because of the `Processed`, say you find a cycle A and you skip it
> >  because it is a loop with max trip count (how do you do that reliably is another story).
> >  There can be a cycle B (in which some nodes of A are contained) that won't be found.
> >  That is a false negative (i.e. we'll say the function is `willreturn` but it might
> >  not be). I think this is a problem.
> >  I'll have to think if that can happen in the CFG, but from the graph theory perspective,
> >  it definitely can.
> >
> > > This new approach also saves the processed nodes so not compute them two times thus enhancing the complexity of the algorithm to be O(N+E).
> > >  The approach also doesn't visit statically unreachable blocks so that also enhances the complexity.
> >
> > Those 2 were true for the previous algorithm as well. If I'm not mistaken, we're doing strictly more work than the previous algorithm (although
> >  the facts you wrote are correct).
>
>
> From what i understaand is that when we have a case like that https://i.imgur.com/cvAjcMa.png and when cycle 1-2-3 have amax trip count and we visit them first we have a problem
>  but i think it won't make a problem since 2 and 3 which are in the bigger cycle must have edges in the cycle in here is the edge of 3 to 4 so 3 and 2 will not be marked processed until they completly finish the 3 node successors so it will complete in the bigger cycle and detect the cycle . I think marking processed at the end will make us escape from a problem like that


In such scenarios you won't have a problem indeed. What I meant is such scenarios: https://imgur.com/a/Ua8xwhg
Consider the case where you find the cycle 0-1-2-0 but it is a loop with max trip count. Upon returning (from recursion) in 1, you will visit 3. But from 3, you won't visit 1 because it's already processed.
If you're doing cycle detection (i.e. check if a graph has a cycle whatsoever), this is correct because there's no point revisiting processed nodes. But this is not our case because we //skip//.

So, to sum up: As I mentioned, `LoopInfo` I think can't be used reliably as it is used now for the points mentioned previously. And we have 2 cases:

1. If we find a reliable way to skip cycles, in this algorithm we will have false negatives which I think is bad. Maybe better the initial version which while it had false positives, it didn't have false negatives.
2. If not, this is probably as good as we can get in cycle detection and has a good complexity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691