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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

Generally it LGTM. I've added some nit comments. Before accepting, I'd like @jdoerfert opinion on the change in `mayContainIrreducibleControl()`.
I think it's a correct change.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2418-2421
+// Helper function that checks whether a function has a cycle,then we check
+// further whether the cycle is a bounded loop cycle.
+// Loops with maximum trip count are considered bounded, any other cycle not.
 // TODO: Replace with more efficent code
----------------
Remove the TODO.
Then please see the comment above.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2426
+  LoopInfo *LI = A.getInfoCache().getAnalysisResultForFunction<LoopAnalysis>(F);
+  // If NoAnalysis is available for the function then we assume any cycle to be
+  // unbounded cycle.
----------------
"If NoAnalysis is available ..." -> "If either SCEV or LoopInfo is not available ..."


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2429
+  // We use scc_iterator which uses Tarjan algorithm to find all the maximal
+  // SCCs. To detect if there's a cycle, you only need to find the maximal ones.
+  if (!SE || !LI) {
----------------
"you only need" -> "we only need"


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2437
+
+  // Check whether there is Irreducible control then the function contains
+  // non-loop cycles.
----------------
If there's irreducible control, the function may contain non-loop cycles.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2442
+
+  // Check whether the loops in the function have maxTripCount and if it has
+  // then it is a bounded loop.
----------------
Any loop that does not have a max trip count is considered unbounded cycle.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691