[llvm-commits] Bug In InstCombine?

[Reid Spencer]

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.

           uint64_t TVA = STO->getZExtValue(), FVA =
SFO->getZExtValue();
           if (isPowerOf2_64(TVA) && isPowerOf2_64(FVA)) {
             // Compute the shift amounts
             unsigned TSA = Log2_64(TVA), FSA = Log2_64(FVA);
             // Construct the "on true" case of the select