[llvm-commits] [llvm] r112539 - /llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp

[Chris Lattner]

On Aug 30, 2010, at 3:07 PM, Owen Anderson wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=112539&view=rev
> Log:
> Fixes and cleanups pointed out by Chris.  In general, be careful to handle 0 results from ComputeValueKnownInPredecessors
> (indicating undef), and re-use existing constant folding APIs.

Thanks,

> @@ -86,6 +87,7 @@
>     virtual void getAnalysisUsage(AnalysisUsage &AU) const {
>       if (EnableLVI)
>         AU.addRequired<LazyValueInfo>();
> +      AU.addPreserved<LazyValueInfo>();
>     }

Does this preserve LVI if LVI is not enabled?  What does "preserving LVI" actually mean?

> @@ -400,12 +399,14 @@
>     // AND or OR of a value with itself is that value.
>     ConstantInt *CI = dyn_cast<ConstantInt>(BO->getOperand(1));
>     if (CI && (BO->getOpcode() == Instruction::And ||
> +        BO->getOpcode() == Instruction::Or)) {
>       SmallVector<std::pair<ConstantInt*, BasicBlock*>, 8> LHSVals;
>       ComputeValueKnownInPredecessors(BO->getOperand(0), BB, LHSVals);
> +      for (unsigned i = 0, e = LHSVals.size(); i != e; ++i)
> +        if (LHSVals[i].first == 0)
> +          Result.push_back(std::make_pair((ConstantInt*)0, LHSVals[i].second));
> +        else if (LHSVals[i].first == CI)
> +          Result.push_back(std::make_pair(CI, LHSVals[i].second));

This isn't safe: undef & 0 = 0, not undef.  Also, why use == here?  Just use ConstantExpr::get.

> @@ -472,17 +473,20 @@
> 
>       // Try to find a constant value for the LHS of an equality 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);
> 
>         ConstantInt *True = ConstantInt::getTrue(I->getContext());
>         ConstantInt *False = ConstantInt::getFalse(I->getContext());
> 
>         for (unsigned i = 0, e = LHSVals.size(); i != e; ++i) {
> -          if (LHSVals[i].first == Cmp->getOperand(1))
> +          if (LHSVals[i].first == 0)
> +            Result.push_back(std::make_pair((ConstantInt*)0,
> +                                            LHSVals[i].second));
> +          else if (ConstantFoldCompareInstOperands(Cmp->getPredicate(), 
> +                                              LHSVals[i].first, 
> +                                              CmpConst))
>             Result.push_back(std::make_pair(True, LHSVals[i].second));
>           else 
>             Result.push_back(std::make_pair(False, LHSVals[i].second));


ConstantFoldCompareInstOperands is overkill, just use ConstantExpr::get.  This also eliminates the need for "True", "False" and fix the miscompilation you just introduced.  (ConstantFoldCompareInstOperands doesn't return bool).

-Chris