[PATCH] D48181: [JumpThreading] Ignore nil destionation when determining whether a block only goes to a single destination

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

My comments are inline. I'm also curious to know what kind of testing you've done on this (`ninja check`, `ninja check-all`, any test-suite runs, etc). I'd like more testing if it's just `ninja check`: the included tests are a small sampling of real-world code and test-suite is a bit better for this kind of change.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1608
     if (isa<UndefValue>(Val))
       DestBB = nullptr;
     else if (BranchInst *BI = dyn_cast<BranchInst>(BB->getTerminator())) {
----------------
It might be better to add a new counter and increment it here:

```
    if (isa<UndefValue>(Val)) {
      ++PredWithUndefDest;
      continue;
    } else if (BranchInst *BI = dyn_cast<BranchInst>(BB->getTerminator())) {
```

I dislike that the logic changes below include  `PredWithKnownDest` for undef (unknown) destinations.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1623
     // If we have exactly one destination, remember it for efficiency below.
-    if (PredToDestList.empty()) {
-      OnlyDest = DestBB;
-      OnlyVal = Val;
-    } else {
-      if (OnlyDest != DestBB)
-        OnlyDest = MultipleDestSentinel;
-      // It possible we have same destination, but different value, e.g. default
-      // case in switchinst.
-      if (Val != OnlyVal)
-        OnlyVal = MultipleVal;
+    // and we can ignore null destination.
+    if (DestBB) {
----------------
Nit: please convert the `.` to a `,` from the end of the previous line or consider re-wording the entire comment.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1655
   // not thread. By doing so, we do not need to duplicate the current block and
   // also miss potential opportunities in case we dont/cant duplicate.
     if (PredWithKnownDest == (size_t)pred_size(BB)) {
----------------
Instead of changing this core logic it might make more sense to do:

```
  if (PredWithUndefDest && !OnlyDest)
    OnlyDest = BB->getTerminator()->getSuccessor(GetBestDestForJumpOnUndef(BB));
  if (OnlyDest && OnlyDest != MultipleDestSentinel) {
    if ((PredWithKnownDest + PredWithUndefDest) == (size_t)pred_size(BB)) {
      ...
```

I say this because `ProcessThreadableEdges()` and the multiple calls from `ProcessBlock()` can create complex and subtle interplay. I try to not change the logic as much as possible between these two.


================
Comment at: test/Transforms/JumpThreading/fold-not-thread.ll:332
+  ret void
+L5:
+  call void @f3()
----------------
Is L5 necessary in this functional test? It's connected to nothing.


Repository:
  rL LLVM

https://reviews.llvm.org/D48181