[PATCH] D24840: Search for all predecessors of loop header while getting loop metadata

[Aditya Kumar via llvm-commits]

hiraditya added inline comments.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:214-215
+    for (BasicBlock *BB : predecessors(getHeader())) {
+      if (!this->contains(BB))
+        continue;
 
----------------
mzolotukhin wrote:
> AFAIU, `this->contains` would be as expensive, as iterating through all blocks (as we did in the original version). Moreover, now we'll do it several times while in the original code we did that only once. Are you sure it's worth it?
> 
> The issue you mentioned - `getLoopID` returns nullptr only after looking into the successors of the first block in the loop - still looks relevant though, so some fix is indeed needed.
> AFAIU, this->contains would be as expensive, as iterating through all blocks
Only for loops with few basic blocks, because DenseBlockSet is a SmallPtrSet, whereas in the previous implementation it would iterate through all the basic blocks and all the successors of each BB in the loop.


https://reviews.llvm.org/D24840