[PATCH] D74691: [Attributor] add some pattern to containsCycle

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I added a bunch of comments. Mostly minor. I think this is almost done and I like how it turned out. Once the comments are addressed I'm fine with this, @baziotis feel free to accept then.

The next step to improve `willreturn` is actually to allow loops with unknown bounds, e.g., link-list traversals but also SCCs that are not loops. During initialize we collect all the ones that might be endless, and in the updateImpl we have to "justify" why they are not. The justification criterion is: they do not contain any synchronizing instruction. `AANoSync` can help with identifying those ;)
@omarahmed Let me know if that is something you want to look into (eventually). If so, we can talk about it more.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2383
+  for (scc_iterator<Function *> I = scc_begin(&F), IE = scc_end(&F); I != IE;
+       ++I) {
+    const std::vector<BasicBlock *> &SCCBBs = *I;
----------------
Nit: Maybe `for (auto &It : make_range(scc_begin(&F), scc_end(&F)))`
Or create a helper `scc_range(..)` that create the range.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2391
+      return true;
     }
+
----------------
Nit: No braces.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2395
+
+    Loop *L = LI->getLoopFor(*BBI);
+    // Check whether the current SCC is a loop or not(Loop is Null).
----------------
Nit: Move the decleration of `SCCBBs` here (where it is used).
Nit: You probably don't need the iterator but just `SCCBBs.front()`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2411
+        else
+          break;
+      }
----------------
Style: No `else` after `return`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2414
+
+    } while ((L = L->getParentLoop()));
   }
----------------
Style: go over the comments and make them all sentences, e.g., capital starting letter, dot at the end, dot in between sentences, etc. 
Nit: Remove `(Loop is Null)`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2415
+    } while ((L = L->getParentLoop()));
   }
   return false;
----------------
I have the feeling there is something missing. What if there is an outer SCC with no loop that is as big as the SCC? I think you need a matching loop with max trip count (not even constant btw.) for each SCC. That is, after the do-loop ended, check if `L` is null or not. If it is, we did not find a matching loop.
(I'm unsure if that can happen wrt. the block order in SCCs but better save than sorry.)


================
Comment at: llvm/test/Transforms/Attributor/willreturn.ll:596
+
+
 attributes #0 = { nounwind uwtable noinline }
----------------
I like the tests a lot. Can we add one or two more with irreducible control, hence SCCs that are not (natural) "loops".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691