[llvm-commits] SETCC InstructionCombining.cpp [#1/n]

[Reid Spencer]

Just confirming .. all changes incorporated.

Reid.

On Wed, 2006-12-20 at 18:22 -0800, Chris Lattner wrote:
> On Dec 17, 2006, at 3:35 PM, Reid Spencer wrote:
> 
> > Chris,
> >
> > Attached is the patch to InstructionCombining.cpp for SETCC conversion
> > to ICmpInst. This passes all tests.
> >
> > All your previous feedback has been incorporated and confirmed. The  
> > test
> > just completed includes all those changes as well.
> >
> > I'm looking forward to finally committing the SETCC patch so we can
> > finish it off and then start removing unnecessary casts and  
> > unifying the
> > integer types.
> 
> +// SimplifyCompare - For a CmpInst this function just orders the  
> operands
> +// so that theyare listed from right (least complex) to left (most  
> complex).
> +// This puts constants before unary operators before binary operators.
> +//
> +bool InstCombiner::SimplifyCompare(CmpInst &I) {
> 
> Please doxygenify the comment.
> 
> +  bool Changed = false;
> +  if (getComplexity(I.getOperand(0)) < getComplexity(I.getOperand 
> (1))) {
> +    I.swapOperands();
> +    Changed = true;
> +  }
> +  // Compare instructions are not associative so there's nothing  
> else we can do.
> +  return Changed;
> +}
> 
> Why not:
> 
>    if (getComplexity(I.getOperand(0)) >= getComplexity(I.getOperand(1)))
>      return false;
>    I.swapOperands();
>    return true;
> 
> ?
> 
> 
> 
>   // isTrueWhenEqual - Return true if the specified setcondinst  
> instruction is
>   // true when both operands are equal...
>   //
>   static bool isTrueWhenEqual(Instruction &I) {
> +  if (ICmpInst *ICI = dyn_cast<ICmpInst>(&I))
> +    return ICI->getPredicate() == ICmpInst::ICMP_EQ  ||
> +           ICI->getPredicate() == ICmpInst::ICMP_UGE ||
> +           ICI->getPredicate() == ICmpInst::ICMP_SGE ||
> +           ICI->getPredicate() == ICmpInst::ICMP_ULE ||
> +           ICI->getPredicate() == ICmpInst::ICMP_SLE;
>     return I.getOpcode() == Instruction::SetEQ ||
>            I.getOpcode() == Instruction::SetGE ||
>            I.getOpcode() == Instruction::SetLE;
>   }
> 
> This should be split into two functions, one for ICmpInst and one for  
> FCmpInst/SetCondInst.  Since your touching it, plz doxygenify also. :)
> 
> 
> 
> @@ -1580,21 +1607,31 @@ static Value *FoldOperationIntoSelectOpe
>     // Figure out if the constant is the left or the right argument.
>     bool ConstIsRHS = isa<Constant>(I.getOperand(1));
>     Constant *ConstOperand = cast<Constant>(I.getOperand(ConstIsRHS));
> 
>     if (Constant *SOC = dyn_cast<Constant>(SO)) {
> -    if (ConstIsRHS)
> -      return ConstantExpr::get(I.getOpcode(), SOC, ConstOperand);
> -    return ConstantExpr::get(I.getOpcode(), ConstOperand, SOC);
> +    if (CmpInst *CI = dyn_cast<CmpInst>(&I)) {
> +      unsigned short pred = CI->getPredicate();
> +      if (ConstIsRHS)
> +        return ConstantExpr::getCompare(pred, SOC, ConstOperand);
> +      return ConstantExpr::getCompare(pred, ConstOperand, SOC);
> 
> This code doesn't appear to be called for compares.  Is it?  If so,  
> the code is broken, you can't just swap the LHS/RHS of a compare like  
> that without breaking things.
> 
> 
> +/// isSignBitCheck - Given an exploded icmp instruction, return true  
> if it
>   /// really just returns true if the most significant (sign) bit is  
> set.
> +static bool isSignBitCheck(ICmpInst::Predicate pred, Value *LHS,
> +                           ConstantInt *RHS) {
> 
> There is no reason to pass LHS into this function (this is an  
> absolute statement, not a bug in your patch) please remove LHS.
> 
> 
> +    case ICmpInst::ICMP_UGE:
> +      // True if LHS u>= RHS and RHS == high-bit-mask (2^7, 2^15,  
> 2^31, etc)
> +      return RHS->getZExtValue() == 1ULL <<
> +        RHS->getType()->getPrimitiveSizeInBits()-1;
> 
> Please add parens to make the precedence more explicit, even though  
> it is right.
> 
> 
> More tomorrow,
> 
> -Chris
> 
>