[llvm-commits] REM Patch

[Reid Spencer]

On Wed, 2006-11-01 at 15:13 -0800, Chris Lattner wrote:
> On Oct 31, 2006, at 3:01 PM, Reid Spencer wrote:
> 
> > Chris,
> >
> > Here's the patch for conversion of Rem -> [USF]Rem instructions.   
> > Please
> > review at your earliest convenience. This passes dejagnu and llvm-test
> > suites.  I've reviewed and modified this patch to make it as simple to
> > review as possible. Fortunately its a lot simpler than DIV.
> 
> Looks good.  Some comments: LangRef says that rem can be applied to  
> vectors, but the asmparser rejects them, please update LangRef.

Done.

> 
> 
> Some comments about instcombine below, prefixed by ***.  After making  
> these changes and testing them, please commit.
> 
> -Chris
> 
> 
> ***This xform (visitURem):
> +    if (Instruction *Op0I = dyn_cast<Instruction>(Op0)) {
> +      // X mul (C1 urem C2) --> 0  iff  C1 urem C2 == 0
> +      if (ConstantExpr::getURem(GetFactor(Op0I), RHS)->isNullValue())
>           return ReplaceInstUsesWith(I, Constant::getNullValue 
> (I.getType()));
>       }
>     }
> 
> *** is doing "(X mul C1) urem C2 --> 0  iff  C1 urem C2 == 0"
> please update the comment.

Done.

> 
> This xform also applies in the srem case, where the code is copied,  
> but the comment unmodified.  Please move it to intcommon.

Done.

>     if (Instruction *RHSI = dyn_cast<Instruction>(I.getOperand(1))) {
> -    // Turn A % (C << N), where C is 2^k, into A & ((C << N)-1)  
> [urem only].
> -    if (I.getType()->isUnsigned() &&
> -        RHSI->getOpcode() == Instruction::Shl &&
> -        isa<ConstantInt>(RHSI->getOperand(0)) &&
> -        RHSI->getOperand(0)->getType()->isUnsigned()) {
> +    // Turn A urem (2^C << N) ->  A & ((C << N)-1) [urem only].
> +    if (RHSI->getOpcode() == Instruction::Shl &&
> +        isa<ConstantInt>(RHSI->getOperand(0))) {
> 
> *** Please change the comment back, your change is not correct.  You  
> can drop 'urem only'.

Done.

> 
> 
> +  // If the top bits of both operands are zero (i.e. we can prove  
> they are
> +  // unsigned inputs), turn this into a urem.
> +  uint64_t Mask = 1ULL << (I.getType()->getPrimitiveSizeInBits()-1);
> +  if (MaskedValueIsZero(Op1, Mask) && MaskedValueIsZero(Op0, Mask)) {
> +    // X srem Y -> X urem Y, iff X and Y don't have sign bit set
> +    return BinaryOperator::createURem(Op0, Op1, I.getName());;
>     }
> *** ";;" -> ";"

Done.

> 
> @@ -4564,41 +4608,24 @@ Instruction *InstCombiner::visitSetCondI
> 
>         // If the first operand is (add|sub|and|or|xor|rem) with a  
> constant, and
>         // the second operand is a constant, simplify a bit.
>         if (BinaryOperator *BO = dyn_cast<BinaryOperator>(Op0)) {
>           switch (BO->getOpcode()) {
> -#if 0
> +        case Instruction::URem:
> +          break;
> *** No need for this case stmt, just remove it.
> 

Done.

>           case Instruction::SRem:
>             // If we have a signed (X % (2^c)) == 0, turn it into an  
> unsigned one.
>             if (CI->isNullValue() && isa<ConstantInt>(BO->getOperand 
> (1)) &&
>                 BO->hasOneUse()) {
>               int64_t V = cast<ConstantInt>(BO->getOperand(1))- 
>  >getSExtValue();
>               if (V > 1 && isPowerOf2_64(V)) {
>                 unsigned L2 = Log2_64(V);
>                 const Type *UTy = BO->getType()->getUnsignedVersion();
>                 Value *NewX = InsertNewInstBefore(new CastInst(BO- 
>  >getOperand(0),
>                                                                UTy,  
> "tmp"), I);
>                 Constant *RHSCst = ConstantInt::get(UTy, 1ULL << L2);
> -              Value *NewRem =InsertNewInstBefore 
> (BinaryOperator::createRem(NewX,
> +              Value *NewRem =InsertNewInstBefore 
> (BinaryOperator::createURem(NewX,
>                                                       RHSCst, BO- 
>  >getName()), I);
>                 return BinaryOperator::create(I.getOpcode(), NewRem,
>                                               Constant::getNullValue 
> (UTy));
>               }
>             }
> *** There is no need to insert the casts here.  It should just  
> replace the srem with a urem in place, no need to change the sign of  
> the inputs.  The old code needed the casts because the sign of the  
> operation was tied to the sign of the inputs.

Right. Done.

Reid.