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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked 3 inline comments as done.
mtrofin added inline comments.


================
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);
+    }
+  }
----------------
kazu wrote:
> 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.
Thanks, this looks neat!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127353