[PATCH] D29620: [JumpThreading] Thread through guards
Sanjoy Das via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 7 10:27:57 PST 2017
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
More information about the llvm-commits
mailing list