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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I think we make good progress here, some comments inlined.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2391
+    // when current SCC has a single block with no loop continue to the next SCC
+    if (!I.hasLoop()) continue;
+
----------------
baziotis wrote:
> You can cache this as you use it 2 times.
Just move this conditional before the `NoAnalysis` check.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2397
+      return true;
+    }
+    if (L->getNumBlocks() > SCCBBs.size()) return true;
----------------
Style: no braces around single statements


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2404-2407
+    if (L->getNumBlocks() > SCCBBs.size()) return true;
+    if (L->getNumBlocks() == SCCBBs.size()) {
+      if (!SE->getSmallConstantMaxTripCount(L)) return true;
+      continue;
----------------
baziotis wrote:
> You want to put those in a loop. You may need to get parent loop multiple times. Something like that.
> ```
> while (L) {
>   if (L is bigger)
>     return true;
>   if (they're equal) {
>     if (there's no max trip count)
>       return true;
>     else
>       continue;
>   }
>   L = getParent();
> }
> ```
And please add a comment block in the beginning of the function or before these size checks explaining what is going on here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691