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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

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

> [Attributor]Detect possibly unbounded cycles in functions
>
> so the new approach detects cycles by using a recursive function that dfs on the CFG


Note that this algorithm detects whether there is //a// cycle in the function (correctl). But now
note that if you find a cycle, you may want to skip it. In that case, it is wrong.
Specifically, because of the `Processed`, say you find a cycle A and you skip it
because it is a loop with max trip count (how do you do that reliably is another story).
There can be a cycle B (in which some nodes of A are contained) that won't be found.
That is a false negative (i.e. we'll say the function is `willreturn` but it might
not be). I think this is a problem.
I'll have to think if that can happen in the CFG, but from the graph theory perspective,
it definitely can.

> This new approach also saves the processed nodes so not compute them two times thus enhancing the complexity of the algorithm to be O(N+E).
>  The approach also doesn't visit statically unreachable blocks so that also enhances the complexity.

Those 2 were true for the previous algorithm as well. If I'm not mistaken, we're doing strictly more work than the previous algorithm (although
the facts you wrote are correct).



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2396
+      Loop *L = LI->getLoopFor(SuccBB);
+      if (!L || !SE->getSmallConstantMaxTripCount(L))
         return true;
----------------
Note that as I mentioned, if a node of a cycle is within a loop, it doesn't mean it is the loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691