[llvm-commits] REM Patch

[Chris Lattner]

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.


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.

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



    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'.


+  // 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());;
    }
*** ";;" -> ";"






@@ -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.

          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.