[PATCH] D127353: FunctionPropertiesAnalysis: handle callsite BBs that loose edges

[Kazu Hirata via Phabricator via llvm-commits]

kazu accepted this revision.
kazu added a comment.
This revision is now accepted and ready to land.

LGTM with some cosmetic suggestions.  Thanks!



================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:73
+  std::deque<const Loop *> Worklist;
+  Worklist.insert(Worklist.end(), LI.begin(), LI.end());
+  while (!Worklist.empty()) {
----------------
```
llvm::append_range(Worklist, LI);
```


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:79-80
+    Worklist.pop_front();
+    Worklist.insert(Worklist.end(), L->getSubLoops().begin(),
+                    L->getSubLoops().end());
+  }
----------------
```
llvm::append_range(Worklist, L->getSubLoops());
```


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:207-212
+  for (const auto *Succ : Successors) {
+    if (!ReIncluded.contains(Succ) && DT.isReachableFromEntry(Succ)) {
+      FPI.reIncludeBB(*Succ);
+      ReIncluded.insert(Succ);
+    }
+  }
----------------
Could we simplify this a little bit like so?

```
  for (const auto *Succ : Successors)
    if (DT.isReachableFromEntry(Succ) && ReIncluded.insert(Succ).second)
      FPI.reIncludeBB(*Succ);
```

We could use a `SetVector` as a uniquified work list and prime it with `Successors` as sentinels, thereby eliminating the two `if` statements in the `while` loop above like so:

```  SetVector<const BasicBlock *> Worklist;

  if (&CallSiteBB != &*Caller.begin()) {
    FPI.reIncludeBB(*Caller.begin());
    Worklist.insert(&*Caller.begin());
  }

  for (const auto *Succ : Successors)
    if (DT.isReachableFromEntry(Succ) && Worklist.insert(Succ))
      FPI.reIncludeBB(*Succ);

  unsigned InitialPos = Worklist.size();
  Worklist.insert(&CallSiteBB);
  for (unsigned I = InitialPos; I != Worklist.size(); I++) {
    const auto *BB = Worklist[I];
    FPI.reIncludeBB(*BB);
    for (const auto *Succ : successors(BB))
      Worklist.insert(Succ);
  }
```

I hear that you have a follow-up patch.  Take this suggestion only if it's compatible with your upcoming changes, of course.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127353