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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

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

> I tried to use loopInfo on the algorithm , I have came with 2 approaches :
>  the first is that if we found a cycle we add the BB in the cycle in a allCyclesBBs vector and after we return from containCycleUtil with all the BBs that makes cycles then loop on the allCyclesBBs vector in the containsCycle function and get the information of the loop with getloopfor as we was doing or ask this inside the containCycleUtil and make this function return us if there was a cycle with no maxTripCount
>  the second is that we do this dfs in a stack style and if we find a cycle we ask for loop info and do the same checks as before like maxtripCount
>  I don't know which of them is better ?


The second one unless I miss something. Oh, I just saw I didn't post the code with `LoopInfo`. But in any case,
you don't need to save the cycles for any reason cycles.

> also the algorithm sometimes in cases moves on BBs 2 times

You probably mean the return label. Yes it does, because you try to find //all// cycles. Compare this to the initial algorithm in which you find if there's any cycle at all (check the `Processed` there that is missing here).

> like this case here :
> 
>   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
>     %tobool2 = icmp ne i32* %n0, null
>     br i1 %tobool2, label %return, label %if.end
>   
>   if.end:                                           ; preds = %entry
>     br label %return
>   
>   return:                                           ; preds = %if.end, %if.then
>     ret i32* %w0
>   }
> 
> 
> so I think if we keep not only the visiting time but the start time = visitingTime and finish time of each node we have visited then we can detect a cycle if we reach a node with no finishTime that means that this node we have visited but not returned from it to make it finish , so this makes the complexity in all the situations O( N+E )



> another question Is it normal to make global variables as I have not found any in the code ?

Generally, global variables are bad. However, when doing recursion, you don't want to pass parameters that are invariant because the fill the stack without a reason
and limit the recursion depth. So, you have 2 choices. Either you make global variables or you make a class that does the thing you want to do.
Then, the global variables get replaced by member variables. To me, this is mostly a matter of preference. Indeed, there are no global variables in the Attributor generally
but in this case, I don't know what would be preferred. This is the easiest thing to replace, so you can use them for now if you want.
In any case, this patch got an unexpected turn so I'd wait for @jdoerfert to tell us an opinion in general. Give him a little a bit of time since:
a) There's a lot to read.
b) He's travelling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691