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

[omar ahmed via Phabricator via llvm-commits]

omarahmed marked 3 inline comments as done.
omarahmed 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;
----------------
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 ?


================
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;
----------------
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 :)


================
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;
----------------
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 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691