[PATCH] D74691: add some pattern to containsCycle

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert requested changes to this revision.
jdoerfert added a comment.
This revision now requires changes to proceed.

Thanks for working on this!

I inlined some high level comments. Once you addressed them you need to look into the test changes. Some of the tests you can automatically update.

With the `opt` tool (and `clang`) on your `PATH` you can run something like

  <llvm-project>/llvm/utils/update_test_checks.py --update-only <llvm-project>/llvm/test/Transforms/Attributor/<TEST>.ll

with the proper paths.

For the auto-updated check lines and the ones you have to manually adjust it is important to verify the new ones are "correct" and in the same spirit as the old ones.

Feel free to ask questions :)



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2371
 // TODO: Replace with more efficent code
-static bool containsCycle(Function &F) {
+static bool containsCycle(Function &F,Attributor &A) {
   SmallPtrSet<BasicBlock *, 32> Visited;
----------------
Here and in other places you need to fix the formatting, e.g., by running clang format on your code. You should configure your programming editor to run clang format on the patch for you. 

I have
```
  function! Formatonsave()
    let l:formatdiff = 1
    pyf /data/src/llvm-project/clang/tools/clang-format/clang-format.py
  endfunction
  autocmd BufWritePre *.h,*.cc,*.cpp call Formatonsave()
```
in my vim configuration. Google should know how that is done for other editors.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2383
+        LoopInfo *LI = 
+          A.getInfoCache().getAnalysisResultForFunction<LoopAnalysis>(F);
+
----------------
You have to check if LI or SE are `null`, that can happen.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2387
+          Loop *L = LI->getLoopFor(BB);
+          if(SE->getSmallConstantMaxTripCount(L)) return false;
+          return true;
----------------
Even if the loop has a constant maximal trip count we cannot assume all loops in the function do. So we need to continue the traversal here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691