[PATCH] D33257: [JumpThreading] Replace uses of Condition safely

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm with nits



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:264
+static void ReplaceFoldableUses(Instruction *Cond, Value *ToVal) {
+  using namespace PatternMatch;
+  assert(Cond->getType() == ToVal->getType());
----------------
`PatternMatch` is unused.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:268
+  // We can unconditionally replace all uses in non-local blocks (i.e. those
+  // strictly dominated by BB), since LVI information is true from the
+  // terminator of BB.
----------------
The non-local blocks need not be strictly dominated by BB, but the uses we replace will be.  The non-local blocks will not be strictly dominated in case of PHI nodes uses:

```
bb1:
  br label %x

bb0:
  assume(%cond)
  br i1 %cond, label %x, label %y

x:
  %v = phi i1 [ %cond, %bb0 ], [ %other_cond, %bb1 ]
```

I.e. the code is correct, but the comment can be more accurate.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:271
+  replaceNonLocalUsesWith(Cond, ToVal);
+  // For the uses within the block, we need to traverse backwards from BB
+  // terminator upto Cond, and replace the current use only if it's guaranteed
----------------
This is a very subjective stylistic thing, but I think this much explanation is unnecessary.  I'd just say something brief like "We only replace uses in instructions that are guaranteed to reach the end of the block, where we know `Cond` is `ToVal`." right before the check on `isGuaranteedToTransferExecutionToSuccessor` -- we have test cases to show why this is needed.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1376
+        else if (OnlyVal && OnlyVal != MultipleVal &&
+                 CondInst->getParent() == BB) {
+          ReplaceFoldableUses(CondInst, OnlyVal);
----------------
Braces unnecessary here.


https://reviews.llvm.org/D33257