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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

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

> [Attributor] Detect possibly unbounded cycles in functions
>
> This patch changes the approach of containsCycle function of looping on
>  the CFG by depth_first to another approach as the depth_first function approach detects if-then-else part in
>  functions as a loop and that result in wrong behaviour, so this approach checks first the analysis of the function if no analysis available for the function then it loops on the SCCs and checkss if the SCC is a cycle or not,this is done with the assumption that any cycle is unbounded.


You don't need anything above here. :)

> If analysis is present for the function then we check for irreducible control to check if there is a non loop cycles if there weren't any then we loop on the loop cycles in the function and check the maxTripCount of the loop.

It would be great to fix grammar / syntax a little bit. You can break sentences to make this easier. Also, you can be a little more specific (when it is useful). For example:
If `LoopInfo` and `SCEV` analyses are available, then:

- We can test whether there is irreducible control (i.e. non-loop cycles). If there is, one of those cycles can be infinite, so we return true.
- Otherwise, loop through all the loops in the function (including child loops) and test whether any of those does not have a maximal trip count (in which case it might be infinite).

Note that the first step can be done more cheaply without requiring `LoopInfo`. But we use it since it is implicitly needed in the second step.

> It also contains some fixed tests and some added tests contain bounded and
>  unbounded loops and non-loop cycles.

This is good, you can leave that.

Aside from the description, we have to do something when analyses are not available. My proposal is to use the old method, again because it doesn't have false negatives and it's reasonably cheap.
Lastly, I want to reiterate that if there's anything that is unclear, please ask.



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


================
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;
----------------
As @jdoerfert mentioned, you can replace this with [[ https://github.com/llvm/llvm-project/blob/master/llvm/lib/Analysis/MustExecute.cpp#L487 | mayContainIrreducibleControl ]]


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691