[llvm-commits] Bug In InstCombine?

[Chris Lattner]

On Mar 5, 2007, at 1:39 PM, Reid Spencer wrote:

> Chris,
>
> While reviewing some of Sheng's patches, I think I found an  
> existing bug
> in InstCombine. Please review this and if its okay I'll commit the
> patch:
>
> Index: InstructionCombining.cpp
> ===================================================================
> RCS
> file: /var/cvs/llvm/llvm/lib/Transforms/Scalar/ 
> InstructionCombining.cpp,v
> retrieving revision 1.647
> diff -t -d -u -p -5 -r1.647 InstructionCombining.cpp
> --- InstructionCombining.cpp    5 Mar 2007 00:11:19 -0000       1.647
> +++ InstructionCombining.cpp    5 Mar 2007 21:37:17 -0000
> @@ -2408,11 +2408,11 @@ Instruction *InstCombiner::visitUDiv(Bin
>    // udiv X, (Select Cond, C1, C2) --> Select Cond, (shr X, C1), (shr
> X, C2)
>    // where C1&C2 are powers of two.
>    if (SelectInst *SI = dyn_cast<SelectInst>(Op1)) {
>      if (ConstantInt *STO = dyn_cast<ConstantInt>(SI->getOperand(1)))
>        if (ConstantInt *SFO = dyn_cast<ConstantInt>(SI->getOperand 
> (2)))
> -        if (!STO->isNullValue() && !STO->isNullValue()) {
> +        if (!STO->isZero() && !SFO->isZero()) {
>
> While I've changed it to use the less expensive isZero, the real  
> bug is
> that this is testing X & X. That is, the second term should check SF0
> against zero, not ST0. I can't think of any reason why testing ST0  
> twice
> would be correct.

Interesting case.  the full code looks like this:

         if (!STO->isNullValue() && !STO->isNullValue()) {
           uint64_t TVA = STO->getZExtValue(), FVA = SFO->getZExtValue 
();
           if (isPowerOf2_64(TVA) && isPowerOf2_64(FVA)) {

The isPowerOf2_64 calls check that the argument is not zero.  As  
such, you can drop the isNullValue checks entirely.

Thanks Reid,

-Chris