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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

In D74691#1893012 <https://reviews.llvm.org/D74691#1893012>, @uenoku wrote:

> Could you make sure that you are using a new pass manager? (It means to use `./opt -passes=attributor ..` but not  `./opt -attributor ..`)
>  If you are using an old pass manager, you might not get `LoopInfo` or other analysis.


Thank you mate, I didn't know I was using the old pass manager.

@omarahmed I would advise you to not work anymore on the old algorithm as it's certainly not effective. I'm sorry, but
I barely have time to work on the new algorithm :/ Otherwise, I would try to answer it.

So, here's a new algorithm with a couple of debug prints:

  static DenseMap<BasicBlock *, unsigned> VisitingTime;
  bool containsCycleUtil(BasicBlock *BB) {
    dbgs() << "Visiting: " << BB->getName() << "\n";
    unsigned VisitingTimeCurrent = VisitingTime[BB];
    for (auto *SuccBB : successors(BB)) {
      dbgs() << BB->getName() << " -> " << SuccBB->getName() << "\n\n";
      unsigned &VisitingTimeSucc = VisitingTime[SuccBB];
      if (!VisitingTimeSucc) {
        VisitingTimeSucc = VisitingTimeCurrent + 1;
        containsCycleUtil(SuccBB);
        VisitingTimeSucc = 0;
      } else {
        dbgs() << "Found cycle: " << VisitingTimeCurrent << ", " << VisitingTimeSucc << "\n";
        assert(VisitingTimeCurrent >= VisitingTimeSucc);
        unsigned CycleSize = VisitingTimeCurrent - VisitingTimeSucc + 1;
        dbgs() << "Cycle size: " << CycleSize << "\n";
      }
    }
    return false;
  }
  
  static bool containsCycle(Attributor &A, Function &F) {
    BasicBlock *EntryBB = &F.getEntryBlock();
    VisitingTime[EntryBB] = 1;
    return containsCycleUtil(EntryBB);
  }

The main modification compared to the last one is that I track visiting time
instead of going back the recursion. This makes the algorithm so much
cleaner (and reliable, for reasons that are not on topic).
Now, this algorithm, AFAICT, finds **all** cycles, **without false positives**. Plus, it detects
only statically reachable cycles. That's good news, it should be strictly better than the initial.

Naively, one could then say "let's apply the same idea as with the SCCs algorithm.
This does not fully work though because check this:

  define void @loop_with_if(i1 %c1, i1 %c2) {
  entry:
    br label %w1
  
  w1:
    br i1 %c1, label %b1, label %exit
  
  b1:
    br label %if
  
  if:
    br i1 %c2, label %t1, label %e1
  
  t1:
    br label %latch
  
  e1:
    br label %latch
  
  latch:
    br label %w1
  
  exit:
    ret void
  }

If you pay attention, it's a `while` with an `if` inside. If you draw out the CFG, you can
see that there's a cycle: w1 -> b1 -> if -> e1 -> latch -> w1
which the algorithm will (correctly) find. The cycle size is 5 but the loop size is 6.
So, in such loops, even if SCEV can deduce a max trip count (which is possible),
the initial method will fail to realize this is one loop.

Which makes a point: There can be a cycle, which is //not// a loop, which still does not
disrupt the maximum trip count guarantee (interestingly, that means that even if
SCCIterator gave us all the SCCs, we would stumble upon this problem).

And there can be many many such real-world cases.
>From this point on, the problem I think starts to get quite complicated. It seems we
need to start saying "if all the blocks of the cycle belong to the same loop and
these blocks contain the header and the latch of the loop blah.. blah.." then
this cycle can be skipped.

IMHO, we should maybe just live with a not-so-perfect-algorithm, or decide
that it is going to take a good amount of time to get it right.

And sorry for the long posts, but this problem turned out to be more complicated
that it initially seemed. I hope I helped a bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691