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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2394
+        Loop *L = LI->getLoopFor(BB);
+        if (L && !SE->getSmallConstantMaxTripCount(L))
+          return true;
----------------
jdoerfert wrote:
> ```
> if (!L || !SE->getSmallConstantMaxTripCount(L))
>   return true;
> ```
> 
> You ignore non-loop cycles otherwise. Please add a non-loop cycle test case, aka. irreducible control flow test case.
> 
> Here is a piece of C that should help create the test:
> ```
> if (foo())
>   goto entry1;
> else
>   goto entry2;
> 
> entry1:
>   if (foo())
>     goto exit;
>   else
>     goto entry2;
> entry2:
>   if (foo())
>     goto exit;
>   else
>     goto entry1;
> exit:
>   return;
> ```
> 
Please note that this code (i.e. the original) is wrong anyway, as for example in this it finds a cycle because
we don't mark nodes unvisited when trying for a new cycle:
```
define i32* @external_sink_ret2_nrw(i32* %n0, i32* %r0, i32* %w0) {
entry:
  %tobool = icmp ne i32* %n0, null
  br i1 %tobool, label %if.end, label %if.then

if.then:                                          ; preds = %entry
  br label %return

if.end:                                           ; preds = %entry
  %0 = load i32, i32* %r0, align 4
  store i32 %0, i32* %w0, align 4
  br label %return

return:                                           ; preds = %if.end, %if.then
  ret i32* %w0
}
```

As I noted in earlier comments, to find all cycles correctly, one has to visit but then unvisit nodes when trying a new cycle.
This eventually led to new algorithms which did full cycle detection and I ended up with this algorithm with the caveats mentioned: https://reviews.llvm.org/D74691/new/#1894286


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691