[llvm-commits] CVS: llvm/lib/Transforms/Scalar/InstructionCombining.cpp

[Chris Lattner]

ComputeMaskedBits had several iterations, here's a review of the one  
in CVS now.  Comments preceeded by ***.

static void ComputeMaskedBits(Value *V, APInt Mask, APInt& KnownZero,
                               APInt& KnownOne, unsigned Depth = 0) {
   assert(V && "No Value?");
   assert(Depth <= 6 && "Limit Search Depth");
   uint32_t BitWidth = Mask.getBitWidth();
   const IntegerType *VTy = cast<IntegerType>(V->getType());
   assert(VTy->getBitWidth() == BitWidth &&

*** VTy is only used in the assertion, please change this to:

   assert(cast<IntegerType>(V->getType())->getBitWidth() == BitWidth &&

..

   KnownZero.clear(); KnownOne.clear();   // Don't know anything.
   APInt KnownZero2(KnownZero), KnownOne2(KnownOne);
   Mask &= APInt::getAllOnesValue(BitWidth);

*** This &= is useless.


   switch (I->getOpcode()) {
...
   case Instruction::Trunc: {
     // All these have integer operands
     uint32_t SrcBitWidth =
       cast<IntegerType>(I->getOperand(0)->getType())->getBitWidth();
     ComputeMaskedBits(I->getOperand(0), Mask.zext(SrcBitWidth),
       KnownZero.zext(SrcBitWidth), KnownOne.zext(SrcBitWidth), Depth 
+1);

*** Please move the nested ZExt's out of line to make this simpler.

     KnownZero.trunc(BitWidth);
     KnownOne.trunc(BitWidth);
     return;
   }
   case Instruction::ZExt:  {
     // Compute the bits in the result that are not present in the  
input.
     const IntegerType *SrcTy = cast<IntegerType>(I->getOperand(0)- 
 >getType());
     APInt NewBits(APInt::getAllOnesValue(BitWidth).shl(SrcTy- 
 >getBitWidth()));

*** NewBits should use SrcBitWidth and should be moved closer/inlined  
into its only use.

     uint32_t SrcBitWidth = SrcTy->getBitWidth();
     ComputeMaskedBits(I->getOperand(0), Mask.trunc(SrcBitWidth),
       KnownZero.trunc(SrcBitWidth), KnownOne.trunc(SrcBitWidth),  
Depth+1);

*** Likewise out of line the trunc's.

     assert((KnownZero & KnownOne) == 0 && "Bits known to be one AND  
zero?");
     // The top bits are known to be zero.
     KnownZero.zext(BitWidth);
     KnownOne.zext(BitWidth);
     KnownZero |= NewBits;
     return;
   }
   case Instruction::SExt: {
     // Compute the bits in the result that are not present in the  
input.
     const IntegerType *SrcTy = cast<IntegerType>(I->getOperand(0)- 
 >getType());
     APInt NewBits(APInt::getAllOnesValue(BitWidth).shl(SrcTy- 
 >getBitWidth()));
*** Likewise, move NewBits down plz, use SrcBitWidth.

     uint32_t SrcBitWidth = SrcTy->getBitWidth();
     ComputeMaskedBits(I->getOperand(0), Mask.trunc(SrcBitWidth),
       KnownZero.trunc(SrcBitWidth), KnownOne.trunc(SrcBitWidth),  
Depth+1);
*** Out of line the truncs plz.

     assert((KnownZero & KnownOne) == 0 && "Bits known to be one AND  
zero?");
     KnownZero.zext(BitWidth);
     KnownOne.zext(BitWidth);

     // If the sign bit of the input is known set or clear, then we  
know the
     // top bits of the result.
     APInt InSignBit(APInt::getSignBit(SrcTy->getBitWidth()));
*** Use SrcBitWidth.

     InSignBit.zext(BitWidth);
*** This zext is dead.

     if ((KnownZero & InSignBit) != 0) {          // Input sign bit  
known zero
       KnownZero |= NewBits;
       KnownOne &= ~NewBits;
*** This &= is dead.
     } else if ((KnownOne & InSignBit) != 0) {    // Input sign bit  
known set
       KnownOne |= NewBits;
       KnownZero &= ~NewBits;
*** This &= is dead.
     } else {                              // Input sign bit unknown
       KnownZero &= ~NewBits;
*** This &= is dead.
       KnownOne &= ~NewBits;
*** This &= is dead.
     }
     return;
   }
   case Instruction::LShr:
     // (ushr X, C1) & C2 == 0   iff  (-1 >> C1) & C2 == 0
     if (ConstantInt *SA = dyn_cast<ConstantInt>(I->getOperand(1))) {
       // Compute the new bits that are at the top now.
       uint64_t ShiftAmt = SA->getZExtValue();
       APInt HighBits(APInt::getAllOnesValue(BitWidth).shl(BitWidth- 
ShiftAmt));
*** Sink HighBits down to its only use.

       // Unsigned shift right.
       Mask <<= ShiftAmt;
       ComputeMaskedBits(I->getOperand(0), Mask,  
KnownZero,KnownOne,Depth+1);
       assert((KnownZero & KnownOne) == 0&&"Bits known to be one AND  
zero?");
       KnownZero = APIntOps::lshr(KnownZero, ShiftAmt);
       KnownOne  = APIntOps::lshr(KnownOne, ShiftAmt);
       KnownZero |= HighBits;  // high bits known zero.
       return;
     }
     break;
   case Instruction::AShr:
     // (ushr X, C1) & C2 == 0   iff  (-1 >> C1) & C2 == 0
*** ushr -> ashr in comment.
     if (ConstantInt *SA = dyn_cast<ConstantInt>(I->getOperand(1))) {
       // Compute the new bits that are at the top now.
       uint64_t ShiftAmt = SA->getZExtValue();
       APInt HighBits(APInt::getAllOnesValue(BitWidth).shl(BitWidth- 
ShiftAmt));
*** Sink HighBits.

       // Signed shift right.
       Mask <<= ShiftAmt;
       ComputeMaskedBits(I->getOperand(0), Mask,  
KnownZero,KnownOne,Depth+1);
       assert((KnownZero & KnownOne) == 0&&"Bits known to be one AND  
zero?");
       KnownZero = APIntOps::lshr(KnownZero, ShiftAmt);
       KnownOne  = APIntOps::lshr(KnownOne, ShiftAmt);

       // Handle the sign bits and adjust to where it is now in the  
mask.
       APInt SignBit(APInt::getSignBit(BitWidth).lshr(ShiftAmt));

       if ((KnownZero & SignBit) != 0) {       // New bits are known  
zero.
         KnownZero |= HighBits;
       } else if ((KnownOne & SignBit) != 0) { // New bits are known  
one.
         KnownOne |= HighBits;
       }
       return;
     }
     break;
   }
}

-Chris