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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2379-2385
+  // If NoAnalysis is available for the function then we assume any cycle to be
+  // unbounded cycle.
+  if (!SE || !LI) {
+    for (scc_iterator<Function *> SCCI = scc_begin(&F); !SCCI.isAtEnd(); ++SCCI)
+      if (SCCI.hasCycle())
         return true;
+    return false;
----------------
omarahmed wrote:
> baziotis wrote:
> > You don't need any of that. This is what was mentioned in a previous diff: Specifically that SCC iterator does not loop through all SCCs, only the top-level one, so you can't do that.
> I tried to do the old method but with no false positives, from what I understand here we wanted only to detect if a cycle exists only and doesn't care if it is a loop or not so I thought that SCCs evenwhen they will give us the top level ones these top level ones will mean that there is a cycle and if there were not these top level SCC structure and it is only consists of single nodes SCCs it will make us not detect a cycle so i used hasCycle function to detect that , does a top level SCC can exist that is not a cycle and a smaller SCC that is a cycle exist in it ?
Sorry, I wrote these comments quite quickly yesterday. You're right. To answer, at first, no, any SCC unless it is single-node without self-loop. Now, `scc_iterator` uses [[ https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm | Tarjan ]], which finds all the //maximal// SCCs. Those are SCCs which are as big as they can get. i.e. in which you can't put any other node and still have an SCC. To detect if there's a cycle, you only need to find the maximal ones.

Also, note that in this case, you could use the algorithm version with `Processed`. This checked whether there is a cycle in the graph in the same complexity as Tarjan, but it's also simpler. Use what you like. However, if you use the `scc_iterator` version, we need a good explanation (you can pick stuff from this comment).


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2388-2394
+  // Check whether there is Irreducible control then the function contains
+  // non-loop cycles.
+  using RPOTraversal = ReversePostOrderTraversal<const Function *>;
+  RPOTraversal FuncRPOT(&F);
+  if (containsIrreducibleCFG<const BasicBlock *, const RPOTraversal,
+                             const LoopInfo>(FuncRPOT, *LI))
+    return true;
----------------
omarahmed wrote:
> baziotis wrote:
> > As @jdoerfert mentioned, you can replace this with [[ https://github.com/llvm/llvm-project/blob/master/llvm/lib/Analysis/MustExecute.cpp#L487 | mayContainIrreducibleControl ]]
> I found that the function is in mustExcute.cpp only and not available in the header file mustExcute.h so how could i use it in this case i guess it should be added to the header :)
Yep, true, it's `static`. Please add it to the header. :)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2398-2399
+  // then it is a bounded loop.
+  for (auto L : LI->getLoopsInPreorder()) {
+    if (!SE->getSmallConstantMaxTripCount(L))
+      return true;
----------------
omarahmed wrote:
> omarahmed wrote:
> > baziotis wrote:
> > > As I said in a previous comment, `getLoopsInPreorder()` (or any way that `LoopInfo` gives you access to all the loops) gives you the //top-level// loops. The fact that a top-level loop has a maximal trip count does not mean that its children loops (i.e. loops entirely contained within this loop) will also have a maximal trip count. So, for every top-level loop, you have to check all its children (i.e. `getSubLoops()`). And all that, in a loop / recursion, because the sub loops may themselves have children loops etc.
> > I have tried it with a test like that
> > 
> > ```
> > for(int i=0;i<n;i++)
> >     for(int j =0;j<n;j++)
> >         for(int k = 0;k<n;k++)
> >              for(int l = 0;l<n;l++)
> > ```
> > and printed the loops that it iterate on and i found it had iterated even on the forth level one , also I have looked on the implementation of the cycle and found that it recursivly was going inside the loops so i guess it goes throw all of the loops not only the top-level ones or I miss something here ?
> I mean the implementation of the function not cycle it is a typo :)
Yes, also true, it gives you all the loops in preorder, I just checked the function. So, yes that should work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691