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

[Michael Zolotukhin via llvm-commits]

mzolotukhin added inline comments.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:214-215
+    for (BasicBlock *BB : predecessors(getHeader())) {
+      if (!this->contains(BB))
+        continue;
 
----------------
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.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:244-245
+  for (BasicBlock *BB : predecessors(getHeader())) {
+    if (!this->contains(BB))
+      continue;
+
----------------
Same question about the cost of `this->contains` versus original iterating over all blocks.


https://reviews.llvm.org/D24840