[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:
> 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.
If you mean that the actual problem is the speed which is a side-effect of conservation, then yes, definitely it's unreasonably slow (given a complete graph, it has sth like exponential time).
Other than that, the fact that a block of a cycle is inside a loop does not mean that the cycle is this loop.
Maybe we can use it if the block is special and it has this specific property. For example, if we knew that the header/latch/... of a loop can't be part of a non-loop cycle. I don't know if something like that holds, I'll try to check tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691