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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691