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