[PATCH] D74691: [Attributor]Detect functions with unbounded loops

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

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

> [Attributor]Detect functions with unbounded loops


Please change that to "[Attributor] Detect possibly unbounded cycles in functions"

> This patch changes the approach of containsCycle function of looping on 
>  the CFG from dfs to bfs as the dfs approach detects if-then-else part in
>  functions as a loop and that result in wrong behaviour, so bfs solves that
>  problem by only moving level by level on the CFG and only detects loops when 
>  there is an edge to a BB in a visited level.
>  This patch also uses maxTripCount to detect unbounded loops in function.
>  It also contains some fixed tests and some added tests contain bounded and
>  unbounded loops.

This message is good, but we will make it better. For now, certainly the old method
is wrong, I just saw it, but also this method is wrong.
First of all, the old method is wrong because of the example you posted. Generally,
that's not a good way to test cycles because the idea is to do a sort of O(n^2), which means
marking nodes visited but then marking them unvisited. You have to do in a kind of recursive
algorithm where as you forward (and deeper in the recursion) you mark them and as you come
back (returning from the recursion) you unmark them. I'll come back to this in a bit.

Regarding your method, consider this case:

  define i32* @test2(i32* %n0, i32* %r0, i32* %w0) {
  entry:
    %tobool = icmp ne i32* %n0, null
    br i1 %tobool, label %l2, label %l1
  
  l1:
    br label %l2
  
  l2:
    br label %return
  
  return:
    ret i32* %w0
  }

Visually (and you can see this with view-cfg) it looks like that: https://imgur.com/a/n8iG7Ig (using my awful drawing skills)
And we construct the branch to first visit the `l2` (by putting in the true label of `br`) so that when `l1` does BFS.
It detects it as a cycle but it's not.
Certainly, BFS is not a popular way to do cycle-detection and I'd abstain from that as all the algorithmic literature
that I know of uses some sort of DFS for this purpose.
So, for now, and we may be able to make it better, you can use something like this:

  bool containsCycle(BasicBlock *BB, SmallPtrSet<BasicBlock *, 32> &Visited, SmallPtrSet<BasicBlock *, 32> &Processed) {
    if (Processed.count(BB)) {
      Processed.erase(BB);
      return false;
    }
    Processed.insert(BB);
    Visited.insert(BB);
    for (auto *SuccBB : successors(BB)) {
      if (!Processed.count(SuccBB) && containsCycle(SuccBB, Visited, Processed))
        return true;
      else if (Visited.count(SuccBB))
        return true;
    }
    Visited.erase(BB);
    return false;
  }

This function works as a utility and you initialize the 2 sets in the main function.
I wrote this quickly but I think it works. Note the usage of the 2 sets:

- `Processed` helps us not do the same work for the same starting BB. It does not help us identify cycles.
- This is done by the `Visited` set. The trick here is that when returning from the recursion, we mark the nodes unvisited, so we can start search in a different tree.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2369-2371
+// Helper function that checks whether a function has unbounded cycle which is
+// a loop that does not have a maximal trip count or any cycle if NoAnalysis is
+// available for the function.
----------------
Here, you want something clearer to what the function does. I'd say, you can go with:
"Helper function that checks whether a function has any cycle which we don't know is bounded.
Loops with maximum trip count are considered bounded, any other cycle not."
We may have to revise this further but it seems a good start.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2373
 // TODO: Replace with more efficent code
-static bool containsCycle(Function &F) {
-  SmallPtrSet<BasicBlock *, 32> Visited;
+static bool containsUnboundedCycle(Function &F, Attributor &A) {
+  bool NoAnalysis = false;
----------------
lebedev.ri wrote:
> I'm a little bit lost here, if a loop is not known to have `SmallConstantMaxTripCount`,
> why does that mean it's unbounded?
It doesn't. In a previous review the name `mayContainUnboundedCycle` was proposed IIRC, which I prefer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691