[PATCH] D28276: [JumpThreading] Fix a bug affecting guards and assumes

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Almost LGTM.  Marking as needing changes to make sure we settle the `replaceDominatedUsesWith` issue before checking in.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:834
+        // from the definition of SSA.
+        if (!CondCmp->use_empty() &&
+            CondCmp->getParent() == BB) {
----------------
Is this clang-formatted?


================
Comment at: lib/Transforms/Utils/Local.cpp:1759
+unsigned llvm::replaceNonLocalUsesWith(Instruction *From, Value *To) {
+  assert(From->getType() == To->getType());
+
----------------
(In a separate change) What do you think about refactoring out a `replaceUsesWithIf` that takes a predicate to decide if a certain use needs to be replaced and change `replaceNonLocalUsesWith` and the two `replaceDominatedUsesWith` s to use it?


================
Comment at: lib/Transforms/Utils/Local.cpp:1764
+  unsigned Count = 0;
+  for (Value::use_iterator UI = From->use_begin(), UE = From->use_end();
+       UI != UE; ) {
----------------
Why not `auto` instead of `Value::use_iterator`?


================
Comment at: lib/Transforms/Utils/Local.cpp:1777
+  return Count;
+
+}
----------------
Stray newline?


================
Comment at: lib/Transforms/Utils/Local.cpp:1809
+  if (auto *FromI = dyn_cast<Instruction>(From))
+    if (FromI->getParent() == BB)
+      return replaceNonLocalUsesWith(FromI, To);
----------------
Please double check, but I don't think this is behavior equivalent.  Today if we have

```
  br <cond>, left, merge

left:
  x = def()
  br merge

merge:
  t = phi(x, 42)
```

then `replaceDominatedUsesWith(x, 100, DT, left)` will not replace the use in the phi node.  But with your change, we will get all of the uses in of `x`, including the one in the phi node.

The real problem here is that `replaceDominatedUsesWith` does not do what it says it does.  Above the use of `x` in the phi node is, in fact, dominated by `left`, but since `replaceDominatedUsesWith` "decays" uses to instructions before checking dominance, that fact is lost.


In any case, if you want to make sure `replaceNonLocalUsesWith` is well tested, I'd nudge you towards adding a test or two in `unittests/Transforms/Utils/Local.cpp`.


================
Comment at: test/Transforms/JumpThreading/assume.ll:89
+  %notnull = icmp ne i32* %array, null
+  call void (i1, ...) @llvm.experimental.guard(i1 %notnull) ["deopt" (i32 0)]
+  br i1 %notnull, label %normal, label %error
----------------
I'd also add a test with an assume followed by a guard.


https://reviews.llvm.org/D28276