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

Stefanos Baziotis via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Mar 8 19:42:07 PDT 2020


baziotis added a comment.

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

> Patch updates :
>
> Check If LoopInfo and SCEV analyses are available, then:
>
> - We can test whether there is irreducible control (i.e. non-loop cycles). If there is, one of those cycles can be infinite, so we return true.
> - Otherwise, loop through all the loops in the function (including child loops) and test whether any of those does not have a maximal trip count (in which case it might be infinite).
>
>   Else if unavailable:
> - We check if any cycle exists then we return true or else we return false.we use scc_iterator to check for cycles.
>
>   Note:
> - scc_iterator uses Tarjan, which finds all the maximal SCCs. Those are SCCs which are as big as they can get. i.e. in which you can't put any other node and still have an SCC. To detect if there's a cycle, you only need to find the maximal ones.


That's quite better I think! :) Side note: You can rephrase or change my proposals regarding documentation, commit messages etc. I'm here only to try to make sure that you get the point across (you're also
free to copy exactly what I say of course, pick whatever you like).

> why it is failing in harbor master remote builds,I am sure it compiles , passes tests and clang format run on it?

On that part unfortunately I can't help. I'm not familiar with harbomaster. AFAIK, it shouldn't fail. Maybe Johannes or @uenoku can help.

> also regarding the tests it gives expected fails =1 in liveness.ll , from what i understand it is expected to fail anyway but is that a problem that needs to be fixed ?

AFAIK, no, this is not a problem. But make sure that you run the //whole// test suite. That is, probably now you run the tests in `llvm/test/Transforms/Attributor/`. After verifying that those pass,
you can run all the tests in `llvm/test`. Note that this should not make a difference since the Attributor is not currently used anywhere. But it's something that should be done in general.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2388-2394
+  // Check whether there is Irreducible control then the function contains
+  // non-loop cycles.
+  using RPOTraversal = ReversePostOrderTraversal<const Function *>;
+  RPOTraversal FuncRPOT(&F);
+  if (containsIrreducibleCFG<const BasicBlock *, const RPOTraversal,
+                             const LoopInfo>(FuncRPOT, *LI))
+    return true;
----------------
omarahmed wrote:
> baziotis wrote:
> > omarahmed wrote:
> > > baziotis wrote:
> > > > As @jdoerfert mentioned, you can replace this with [[ https://github.com/llvm/llvm-project/blob/master/llvm/lib/Analysis/MustExecute.cpp#L487 | mayContainIrreducibleControl ]]
> > > I found that the function is in mustExcute.cpp only and not available in the header file mustExcute.h so how could i use it in this case i guess it should be added to the header :)
> > Yep, true, it's `static`. Please add it to the header. :)
> i tried to erase static as static means that it will only used in the cpp file also i have put the definition in the header file but that resulted in an error in linking so how could we do that here ? I guess also that all the functions i saw was only static in the cpp file or they are belong to a class in the .h or cpp file but no function that is implemented alone so I think that getting this 3 lines is more clean or what do you think ?
I think it's better if we use the already made function. I'll try to do that myself tomorrow and see what problems come up.

[Irrelevant] Some more info about `static`: It means that you can use the function only in this //compilation unit//. Usually, a compilation unit is a `.cpp` file, but if you include a `.cpp` file in another, those 2 will make //one// compilation unit.
As a side note, `static` has more to do with linking than with compiling. It basically means that the symbol (i.e. the function, which in assembly is just a label) is not exported. So, no code from another object file can jump to it.
Here's a great video in case you're interested in this kind of stuff: https://www.youtube.com/watch?v=n4fI4eUTTKM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691





More information about the llvm-commits mailing list