[PATCH] D74691: [Attributor] add some pattern to containsCycle

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D74691#1883373 <https://reviews.llvm.org/D74691#1883373>, @baziotis wrote:

> In D74691#1883317 <https://reviews.llvm.org/D74691#1883317>, @jdoerfert wrote:
>
> > In D74691#1883013 <https://reviews.llvm.org/D74691#1883013>, @omarahmed wrote:
> >
> > > [...]
> > >  as the code puts on this function willreturn attribute and in the file read_write_returned_arguments_scc.ll it doesn't contain willreturn attribute so it gives an error with filecheck
> >
> >
> > It should have. Your extension to `AAWillReturn` should generally result in more `willreturn` attributes being deduced. That is the point after all. We need to verify each change to the CHECK lines though. We should also write explicit tests for loops with and without upper trip count limits, assuming we don't have them already.
>
>
> I just saw the test, and assuming I don't miss anything inter-procedural that happens, the previous version of `containsCycle()` should have been able to deduce that this does not have a loop.


I think the algorithm was broken and detected `if-then-else` as potential cycle because the merge block was visited twice.

That said, I was hoping/expecting for more test changes with this. Can we write some explicit tests and add them to the `willreturn.ll` file?



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2372
 // TODO: Replace with more efficent code
-static bool containsCycle(Function &F) {
-  SmallPtrSet<BasicBlock *, 32> Visited;
-
-  // Traverse BB by dfs and check whether successor is already visited.
-  for (BasicBlock *BB : depth_first(&F)) {
-    Visited.insert(BB);
-    for (auto *SuccBB : successors(BB)) {
-      if (Visited.count(SuccBB))
+static bool containsCycle(Function &F, Attributor &A) {
+  bool NoAnalysis = false;
----------------
The name should now be something like `mayContainUnboundedCycle`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2430
 // FIXME: Any cycle is regarded as endless loop for now.
 //        We have to allow some patterns.
+static bool containsPossiblyEndlessLoop(Function *F, Attributor &A) {
----------------
The fixme is outdated, remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691