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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2394
+        Loop *L = LI->getLoopFor(BB);
+        if (L && !SE->getSmallConstantMaxTripCount(L))
+          return true;
----------------
baziotis wrote:
> 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
Not wrong but too conservative ;) That's a big difference. However, I agree, we should be as precise as is reasonably cheap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691