[llvm-commits] [llvm] r112589 - in /llvm/trunk: lib/Transforms/Scalar/JumpThreading.cpp test/Transforms/JumpThreading/2010-08-31-InfiniteRecursion.ll
Chris Lattner
clattner at apple.com
Tue Aug 31 10:22:42 PDT 2010
On Aug 31, 2010, at 12:36 AM, Owen Anderson wrote:
> Author: resistor
> Date: Tue Aug 31 02:36:34 2010
> New Revision: 112589
>
> URL: http://llvm.org/viewvc/llvm-project?rev=112589&view=rev
> Log:
> More Chris-inspired JumpThreading fixes: use ConstantExpr to correctly constant-fold undef, and be more careful with its return value.
> This actually exposed an infinite recursion bug in ComputeValueKnownInPredecessors which theoretically already existed (in JumpThreading's
> handling of and/or of i1's), but never manifested before. This patch adds a tracking set to prevent this case.
Thanks.
> @@ -270,12 +271,17 @@
> ///
> bool JumpThreading::
> ComputeValueKnownInPredecessors(Value *V, BasicBlock *BB,PredValueInfo &Result){
> + if (!RecursionSet.insert(std::make_pair(V, BB)).second)
> + return false;
Please add a comment explaining what this is about.
> +
> // If V is a constantint, then it is known in all predecessors.
> if (isa<ConstantInt>(V) || isa<UndefValue>(V)) {
> ConstantInt *CI = dyn_cast<ConstantInt>(V);
>
> for (pred_iterator PI = pred_begin(BB), E = pred_end(BB); PI != E; ++PI)
> Result.push_back(std::make_pair(CI, *PI));
> +
> + RecursionSet.erase(std::make_pair(V, BB));
> return true;
You do this in many places throughout the function, please replace the insert/erase pair with an RAII object. Otherwise, it will certainly get unbalanced someday.
> @@ -382,38 +399,48 @@
> // Try to simplify some other binary operator values.
> } else if (BinaryOperator *BO = dyn_cast<BinaryOperator>(I)) {
> ConstantInt *CI = dyn_cast<ConstantInt>(BO->getOperand(1));
> + if (CI) {
You can put the dyn_cast in the 'if'.
> SmallVector<std::pair<ConstantInt*, BasicBlock*>, 8> LHSVals;
> ComputeValueKnownInPredecessors(BO->getOperand(0), BB, LHSVals);
> +
> + // Try to use constant folding to simplify the binary operator.
> + for (unsigned i = 0, e = LHSVals.size(); i != e; ++i) {
> + Constant *Folded = 0;
> if (LHSVals[i].first == 0) {
> + Folded = ConstantExpr::get(BO->getOpcode(),
> + UndefValue::get(BO->getType()),
> + CI);
> + } else {
> + Folded = ConstantExpr::get(BO->getOpcode(), LHSVals[i].first, CI);
> }
Please factor this better, you're duplicating a bunch of code. The key part of this is something like:
Value *V = LHSVals[i].first ? LHSVals[i].first : UndefValue::get(BO->getType());
Strive for minimality, if you find yourself duplicating code, "something is wrong".
> + // Try to find a constant value for the LHS of a comparison,
> // and evaluate it statically if we can.
> if (Constant *CmpConst = dyn_cast<Constant>(Cmp->getOperand(1))) {
> SmallVector<std::pair<ConstantInt*, BasicBlock*>, 8> LHSVals;
> ComputeValueKnownInPredecessors(I->getOperand(0), BB, LHSVals);
>
> for (unsigned i = 0, e = LHSVals.size(); i != e; ++i) {
> + Constant * Folded = 0;
> if (LHSVals[i].first == 0)
> + Folded = ConstantExpr::getCompare(Cmp->getPredicate(),
> + UndefValue::get(CmpConst->getType()), CmpConst);
> + else
> + Folded = ConstantExpr::getCompare(Cmp->getPredicate(),
> + LHSVals[i].first, CmpConst);
Needless repetition again.
> +
> + if (ConstantInt *FoldedCInt = dyn_cast<ConstantInt>(Folded))
> + Result.push_back(std::make_pair(FoldedCInt, LHSVals[i].second));
> + else if (isa<UndefValue>(Folded))
> + Result.push_back(std::make_pair((ConstantInt*)0,LHSVals[i].second));
This pattern is occurring enough that it is probably also worth factoring out to a static helper function.
> }
>
> + RecursionSet.erase(std::make_pair(V, BB));
> return !Result.empty();
> }
> }
> @@ -505,9 +540,11 @@
> Result.push_back(std::make_pair(CInt, *PI));
> }
>
> + RecursionSet.erase(std::make_pair(V, BB));
> return !Result.empty();
> }
>
> + RecursionSet.erase(std::make_pair(V, BB));
> return false;
> }
Ick.
> @@ -1126,8 +1163,9 @@
> return false;
>
> SmallVector<std::pair<ConstantInt*, BasicBlock*>, 8> PredValues;
> - if (!ComputeValueKnownInPredecessors(Cond, BB, PredValues))
> + if (!ComputeValueKnownInPredecessors(Cond, BB, PredValues)) {
> return false;
> + }
this isn't needed :)
> +++ llvm/trunk/test/Transforms/JumpThreading/2010-08-31-InfiniteRecursion.ll Tue Aug 31 02:36:34 2010
I appreciate the testcase, but:
1) please hand minimizing it. Bugpoint is a useful tool to get the testcase started, it is not the one true answer for providing testcases you can just throw in the testsuite.
2) when you get it smaller, please add it to Transforms/JumpThreading/crash.ll
3) I'm still waiting for testcases for all the other stuff you added to jump threading. You claim that they do useful things, prove it with minimal testcases. There are lots of examples showing this for other things in jumpthreading, it is *very* important that you add them. If you don't, I will eventually come back and rip out the code, see that nothing breaks in the testsuite, and we'll regress.
Please document every major addition to jump threading (and any other optimizer) with a minimal testcase.
-Chris
> @@ -0,0 +1,91 @@
> +; RUN: opt < %s -jump-threading -disable-output
> +; ModuleID = 'bugpoint-reduced-simplified.bc'
> +target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
> +target triple = "x86_64-apple-darwin10.4"
> +
> +define void @encode_one_macroblock_highfast() nounwind ssp {
> +entry:
> + switch i32 undef, label %bb13 [
> + i32 1, label %bb10
> + i32 2, label %bb12
> + ]
> +
> +bb10: ; preds = %entry
> + unreachable
> +
> +bb12: ; preds = %entry
> + unreachable
> +
> +bb13: ; preds = %entry
> + br i1 undef, label %bb137, label %bb292
> +
> +bb137: ; preds = %bb13
> + br i1 undef, label %bb150, label %bb154
> +
> +bb150: ; preds = %bb137
> + unreachable
> +
> +bb154: ; preds = %bb137
> + br i1 undef, label %bb292, label %bb246
> +
> +bb246: ; preds = %bb154
> + br i1 undef, label %bb292, label %bb247
> +
> +bb247: ; preds = %bb246
> + br i1 undef, label %bb248, label %bb292
> +
> +bb248: ; preds = %bb247
> + br i1 undef, label %bb249, label %bb292
> +
> +bb249: ; preds = %bb248
> + br i1 undef, label %bb254, label %bb250
> +
> +bb250: ; preds = %bb249
> + unreachable
> +
> +bb254: ; preds = %bb249
> + br i1 undef, label %bb292, label %bb255
> +
> +bb255: ; preds = %bb288.bb289.loopexit_crit_edge, %bb254
> + br i1 undef, label %bb.nph.split.us, label %bb269
> +
> +bb.nph.split.us: ; preds = %bb255
> + br i1 undef, label %bb.nph.split.us.split.us, label %bb269.us.us31
> +
> +bb.nph.split.us.split.us: ; preds = %bb.nph.split.us
> + br i1 undef, label %bb269.us.us, label %bb269.us.us.us
> +
> +bb269.us.us.us: ; preds = %bb287.us.us.us, %bb.nph.split.us.split.us
> + %indvar = phi i64 [ %indvar.next, %bb287.us.us.us ], [ 0, %bb.nph.split.us.split.us ] ; <i64> [#uses=1]
> + %0 = icmp eq i16 undef, 0 ; <i1> [#uses=1]
> + br i1 %0, label %bb287.us.us.us, label %bb286.us.us.us
> +
> +bb287.us.us.us: ; preds = %bb269.us.us.us
> + %indvar.next = add i64 %indvar, 1 ; <i64> [#uses=2]
> + %exitcond = icmp eq i64 %indvar.next, 4 ; <i1> [#uses=1]
> + br i1 %exitcond, label %bb288.bb289.loopexit_crit_edge, label %bb269.us.us.us
> +
> +bb286.us.us.us: ; preds = %bb269.us.us.us
> + unreachable
> +
> +bb269.us.us: ; preds = %bb287.us.us, %bb.nph.split.us.split.us
> + br i1 undef, label %bb287.us.us, label %bb286.us.us
> +
> +bb287.us.us: ; preds = %bb269.us.us
> + br i1 undef, label %bb288.bb289.loopexit_crit_edge, label %bb269.us.us
> +
> +bb286.us.us: ; preds = %bb269.us.us
> + unreachable
> +
> +bb269.us.us31: ; preds = %bb.nph.split.us
> + unreachable
> +
> +bb269: ; preds = %bb255
> + unreachable
> +
> +bb288.bb289.loopexit_crit_edge: ; preds = %bb287.us.us, %bb287.us.us.us
> + br i1 undef, label %bb292, label %bb255
> +
> +bb292: ; preds = %bb288.bb289.loopexit_crit_edge, %bb254, %bb248, %bb247, %bb246, %bb154, %bb13
> + unreachable
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list