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
