[PATCH] D29620: [JumpThreading] Thread through guards

[Sanjoy Das via Phabricator via llvm-commits]

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

Comments inline.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2053
+    Predecessors.push_back(Pred);
+    if (Predecessors.size() > 2)
+      return false;
----------------
This matching is a bit odd -- how about:

```
BasicBlock *Pred0, *Pred1;
auto PI = pred_beg(BB), PE = pred_end(BB);
if (PI == PE)
  return false;
Pred0 = *PI++;
if (PI == PE)
  return false;
Pred1 = *PI++;
if (PI != PE)
  return false;
```



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2060
+    return false;
+
+  BasicBlock *Pred1 = Predecessors[0];
----------------
I think you also need to check that these preds have only one successor.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2084
+  Value *BranchCond = BI->getCondition();
+  BasicBlock *TrueBranch = BI->getSuccessor(0);
+  BasicBlock *FalseBranch = BI->getSuccessor(1);
----------------
Let's call these `TrueDest` and `FalseDest`.  `TrueBranch` sounds like it is a `BranchInst`.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2095
+  bool ImpliedTrueBranch = *Implication;
+  BasicBlock *UnguardedBlock = ImpliedTrueBranch ? TrueBranch : FalseBranch;
+  BasicBlock *GuardedBlock = ImpliedTrueBranch ? FalseBranch : TrueBranch;
----------------
I'm not sure this is correct if `ImpliedTrueBranch` is false.  By contract, `ImpliedTrueBranch` being false means: `BranchCond ==> NOT(GuardCond)`, but here you're using it as `NOT(BranchCond) ==> GuardCond`, which isn't the same thing.


For instance, say `BranchCond` is `A < 10`, and `GuardCond` is `A > 20`.  Then `NOT(GuardCond)` is `A <= 20`, and `BranchCond ==> NOT(GuardCond)`.  But that does not mean that you can eilde the guard on the false destination of the branch -- on the false destination all you know is `NOT(A < 10)` == `A >= 10`, but that's not sufficient to elide a guard on `A > 20` since `A` could be `11`.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2104
+  // Duplicate all instructions before the guard in the unguarded branch.
+  UnguardedBlock =
+      DuplicateInstructionsInSplitBetween(BB, UnguardedBlock, Guard,
----------------
I think we need a cap on how many instructions we duplicate this way.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2110
+  GuardedBlock =
+      DuplicateInstructionsInSplitBetween(BB, GuardedBlock, AfterGuard,
+                                          GuardedMapping);
----------------
There are certain instructions that cannot be duplicated (see `CallInst::cannotDuplicate`), so you should bail out if you see one of those here.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2140
+BasicBlock *JumpThreadingPass::DuplicateInstructionsInSplitBetween(
+    BasicBlock *BB, BasicBlock *PredBB, Instruction *StopAt,
+    DenseMap<Instruction *, Value *> &ValueMapping) {
----------------
In a later change, let's add this to Cloning.h with some unit tests of its own.


https://reviews.llvm.org/D29620