[PATCH] D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:369
   FindLoopHeaders(F);
-
-  bool Changed;
+  bool Changed, EverChanged = false;
   do {
----------------
I'd put it on a separate line, one uninitialized value looks a bit suspicious.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:372
     Changed = false;
-    for (Function::iterator I = F.begin(), E = F.end(); I != E;) {
+    for (Function::iterator I = F.begin(), E = F.end(); I != E; ++I) {
       BasicBlock *BB = &*I;
----------------
Can this be rewritten to a range-based for loop?


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:362
+  // waste of compute time and can potentially lead to hangs.
+  UnreachableBlocks.clear();
+  DominatorTree &DT = DDT->flush();
----------------
I can see that `UnreachableBlocks` are being cleared every time at the beginning and at the end. Can it be a local function variable instead, or am I missing something? 


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:378
         Changed = true;
-
-      ++I;
-
-      // Don't thread branches over a block that's slated for deletion.
-      if (DDT->pendingDeletedBB(BB))
+      if (BB == &F.getEntryBlock() || DDT->pendingDeletedBB(BB))
         continue;
----------------
Can you add a comment on why the entry block is skipped? I guess this is because the latter code tries to delete block with no predecessor as it figures they are unreachable, but that's not the case for the function entry?


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:395
+      // is "almost empty", we attempt to merge BB with its sole successor.
       BranchInst *BI = dyn_cast<BranchInst>(BB->getTerminator());
       if (BI && BI->isUnconditional() &&
----------------
I think the type is obvious here: `auto *BI = ...`


https://reviews.llvm.org/D44177