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