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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2369-2370
 
-// Helper function that checks whether a function has any cycle.
+// Helper function that checks whether a function has any cycle and check if
+// the cycle have an upper bound or not.
 // TODO: Replace with more efficent code
----------------
I think this doc is a little bit misleading. Maybe better `Helper function that checks whether a function has any unbounded cycle`. Actually, given that you change the name to what Johannes proposed, an even better doc I think is: `An unbounded cycle in this case is either a loop that does not have a maximal trip count or any other cycle`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2406-2407
+    // Compare the number of blocks of SCC and the loop and check :
+    // When the loop have more number of blocks then the current SCC is not
+    // a loop.
+    // When they are equal then it is a loop and check the maxTripCount if it
----------------
I'd propose that you add a little bit more here like `... is not a loop since getLoopFor() gives us initially the innermost loop. Then, with getParentLoop(), we move strictly from innermost to outermost. Since a loop is always an SCC, if this SCC was a loop, we would match exactly the number of blocks in one of this loops. If the number of blocks is less, then this SCC is a non-loop cycle inside one of these loops (or just a cycle if no loop exists)`.
That's maybe quite verbose, if you think something more concise, you may add it. But in any case, I think this is a subtle point that should not be left without explanation. @jdoerfert what do you think?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2410
+    // has an upper bound.
+    // Else we get the parent loop and check again.
+    do {
----------------
Why? This another subtle point. I think you can use some of the initial description I had written if you think it's good.

Note: As a general rule of thumb, in docs, try to comment //why// something is done rather //what// is done. The what is usually obvious while the why a lot of times not.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2419
+        else
+          continue;
+      }
----------------
I believe here you want to continue to the next SCC but rather you continue to the next parent loop. In this case, since this do-while is the last thing in the SCC loop, you can just break.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2421
+      }
+    } while (L != L->getParentLoop() && (L = L->getParentLoop()));
   }
----------------
Sorry but this is unclear to me. Could you explain the reasoning? I'd think that the `while` version is easier to deal with, was there a problem with it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691