[PATCH] D29620: [JumpThreading] Thread through guards

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

Preparing fixes.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2060
+    return false;
+
+  BasicBlock *Pred1 = Predecessors[0];
----------------
sanjoy wrote:
> I think you also need to check that these preds have only one successor.
This is not required. Imagine
         A
       /    \
      B      C
    /  \    /  \
  D      E       F


Hoisting instructions before guards to split blocks between B-E, C-E looks valid. We don't break any look structures in this case. It is kinda equivalent to splitting E into 2 parts (E1 and E2) and then duplicating E1 to E1a and E1b. As result, we have something like

          A
       /      \
      B         C
    /  \      /  \
  D     E1a E1b   F
          \ /
           E2

That looks valid for me. Please let me know if you see a problem in doing so.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2095
+  bool ImpliedTrueBranch = *Implication;
+  BasicBlock *UnguardedBlock = ImpliedTrueBranch ? TrueBranch : FalseBranch;
+  BasicBlock *GuardedBlock = ImpliedTrueBranch ? FalseBranch : TrueBranch;
----------------
sanjoy wrote:
> 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`.
> 
You're right, it is only valid to do that if BranchCond = NOT(GuardCond). Thanks for noticing this!


https://reviews.llvm.org/D29620