[PATCH] D74691: [Attributor] Detect SCCs with unbounded cycles

[omar ahmed via Phabricator via llvm-commits]

omarahmed added a comment.

In D74691#1888455 <https://reviews.llvm.org/D74691#1888455>, @baziotis wrote:

> In D74691#1888442 <https://reviews.llvm.org/D74691#1888442>, @omarahmed wrote:
>
> > In D74691#1888396 <https://reviews.llvm.org/D74691#1888396>, @baziotis wrote:
> >
> > > @omarahmed @jdoerfert
> > >
> > > I'm in the unpleasant position to tell you that the method I proposed is wrong. `SCCIterator` uses Tarjan's algorithm which for some reason, I remembered it finds all strongly-connected components but actually, it finds only maximal.
> > >  That means, if a loop has an SCC inside it, we'll never find it since the loop is the maximal SCC. That probably means that we have to fall-back to the method that Johannes had proposed:
> > >
> > >   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.
> > >
> > >
> > > I'm really sorry for that. I'll try to think if anything better can be done.
> > >
> > > Edit: Let's not forget of course: A big thanks to @Meinersbur for pointing this out.
> >
> >
> > that's fine , no problem :D
> >  I think i can move faster now as i know the small mistakes i have done till now from the great reviews :)
> >
> > But if it finds only the maximum ones why it hadn't returned willreturn here , as it shouldn't have seen the while inside
> >
> >   ; int non_loop_inside_loop(int n) {
> >   ;   int ans = 0;
> >   ;   for (int i = 0; i < n; i++) {
> >   ;     while (1)
> >   ;       ans++;
> >   ;   }
> >   ;   return ans;
> >   ; } 
> >
>
>
> It's good to refer to the LLVM IR, rather than the C/C++ because they can have multiple LLVM IR translations. Just to be sure we're talking about the same thing. :)
>  In this case, I assume you meant the relevant test case in `willreturn.ll`. If you put it in a file on its own and run it through the Attributor, with a couple of prints, you'll
>  see that `NoAnalysis` is `true` (btw, I recommend that you try to answer questions by following the code - it's always a learning experience).
>  I don't know why and that's a good question (on another topic, but still). Interestingly, both `LI` and `SE` are `null`.  So, for some reason, for this function, we couldn't
>  get either of those analyses. I'll try to find some time to check tomorrow.


that's a great tip,thank you, you are right :)

I


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691