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. _______________________________________________ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits