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

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

Hi @kuhar , thanks for the review.  As a general comment I tried to minimize changes to JumpThreading to remove `removeUnreachableBlocks`.



================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:369
   FindLoopHeaders(F);
-
-  bool Changed;
+  bool Changed, EverChanged = false;
   do {
----------------
kuhar wrote:
> I'd put it on a separate line, one uninitialized value looks a bit suspicious.
Will do. `Changed` is set 2 lines later but I don't mind either way.


================
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;
----------------
kuhar wrote:
> Can this be rewritten to a range-based for loop?
Possibly. I left it as it was to minimize disruptions. I wasn't certain if this form handles deleted elements better than `for (auto &BB : F)` does.  I can give it a try and see if all tests pass.


================
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();
----------------
kuhar wrote:
> 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? 
I added `UnreachableBlocks` to `JumpThreading.h` because I used the `LoopHeaders` code as my model. Originally I was tracking `ValidBlocks` but that became memory and performance prohibitive. I can try `UnreachableBlocks` as a local variable although I'm not quite sure what the LLVM style guide is for defining a local variable differently based on `NDEBUG`.

Either way this `UnreachableBlocks.clear()` should be removed.


================
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;
----------------
kuhar wrote:
> 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?
It's the same code as before. If you look at the original JumpThreading loop both of the subsequent cleanups (`DeleteDeadBlock` and `TryToSimplifyUncondBranchFromEmptyBlock`) guarded against touching the entry. This is because both of these delete `BB` block from`F`. Whether or not this is pessimizing potential JumpThreads is uncertain.  I can certainly add a comment.


================
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() &&
----------------
kuhar wrote:
> I think the type is obvious here: `auto *BI = ...`
Will do.


https://reviews.llvm.org/D44177