[llvm-commits] SETCC InstructionCombining.cpp [#2/3]
Chris Lattner
clattner at apple.com
Fri Dec 22 12:08:31 PST 2006
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.
Onward. There are some serious bugs here that need to be addressed.
Overall the patch is looking good though, I'll get you 3/3 in the
next few hours.
-Chris
@@ -2177,18 +2225,18 @@ Instruction *InstCombiner::visitMul(Bina
if (isa<ConstantInt>(SCIOp1) &&
- isSignBitCheck(SCI->getOpcode(), SCIOp0, cast<ConstantInt>
(SCIOp1))) {
+ isSignBitCheck(SCI->getPredicate(), SCIOp0,
cast<ConstantInt>(SCIOp1))) {
// Shift the X value right to turn it into "all signbits".
Beware 80 columns.
+/// S<=> Definition S<=> Definition
+/// 0000 Always false 1000 Alwasy false
typo alwasy
+/// Four bits are used to represent the condition, as follows:
+/// 0 A > B
+/// 1 A == B
+/// 2 A < B
+/// 3 A and B are signed
+///
+/// S<=> Definition S<=> Definition
+/// 0000 Always false 1000 Alwasy false
+/// 0001 A u> B 1001 A s> B
+/// 0010 A == B 1010 A == B
+/// 0011 A u>= B 1011 A s>= B
+/// 0100 A u< B 1100 A s< B
+/// 0101 A != B 1101 A != B
+/// 0110 A u<= B 1110 A s<= B
+/// 0111 Always true 1111 Always true
+///
+static unsigned getICmpCode(const ICmpInst *ICI) {
This table (and it's use) is not correct. It will miscompile:
(a <u b) | (a <s b)
You really want separate bits for s<, s>, u<, u>. In this case,
you'd detect that the merged comparison can't be done with a single
compare, so you give up.
+/// getICmpValue - This is the complement of getICmpCode, which
turns an
/// opcode and two operands into either a constant true or false,
or a brand new
+/// ICmp instruction.
+static Value *getICmpValue(unsigned Opcode, Value *LHS, Value *RHS) {
switch (Opcode) {
+ case 0:
+ case 8: return ConstantBool::getFalse();
+ case 1: return new ICmpInst(ICmpInst::ICMP_UGT, LHS, RHS);
+ case 9: return new ICmpInst(ICmpInst::ICMP_SGT, LHS, RHS);
+ case 10:
+ case 2: return new ICmpInst(ICmpInst::ICMP_EQ, LHS, RHS);
+ case 3: return new ICmpInst(ICmpInst::ICMP_UGE, LHS, RHS);
+ case 11: return new ICmpInst(ICmpInst::ICMP_SGE, LHS, RHS);
+ case 4: return new ICmpInst(ICmpInst::ICMP_ULT, LHS, RHS);
+ case 12: return new ICmpInst(ICmpInst::ICMP_SLT, LHS, RHS);
+ case 13:
+ case 5: return new ICmpInst(ICmpInst::ICMP_NE, LHS, RHS);
+ case 6: return new ICmpInst(ICmpInst::ICMP_ULE, LHS, RHS);
+ case 14: return new ICmpInst(ICmpInst::ICMP_SLE, LHS, RHS);
+ case 7:
+ case 15: return ConstantBool::getTrue();
+ default: assert(0 && "Illegal ICmp code!"); return 0;
Please move the default case to the top and drop the 'return 0'.
That way in release-assert builds, the default case will fall into
the first case, which will produce slightly more efficient code.
Instruction *InstCombiner::InsertRangeTest(Value *V, Constant *Lo,
Constant *Hi,
+ bool isSigned, bool Inside,
+ Instruction &IB) {
Your changes to the callers of InsertRangeTest are not correct. You
will fold things like:
'x >= -14 <u 27' incorrectly. You should only fold if the sign
of both halves is the same.
Inside InsertRangeTest you have:
+ ICmpInst::Predicate pred = (isSigned ?
ICmpInst::ICMP_SLE:ICmpInst::ICMP_ULE);
+ assert(cast<ConstantBool>(ConstantExpr::getICmp(pred, Lo, Hi))-
>getValue() &&
"Lo is not <= Hi in range emission code!");
Please sink 'pred' into the assert. I don't care how you write it,
but I don't want 'pred' to be a long-lived variable like this. This
makes it clear that the value of pred isn't retained, which makes the
code easier to read.
+ pred = (isSigned ? ICmpInst::ICMP_SLT : ICmpInst::ICMP_ULT);
+ // V >= Min && V < Hi --> V < Hi
+ if (cast<ConstantIntegral>(Lo)->isMinValue(isSigned))
+ return new ICmpInst(pred, V, Hi);
Likewise, this should be something like:
+ // V >= Min && V < Hi --> V < Hi
+ if (cast<ConstantIntegral>(Lo)->isMinValue(isSigned)) {
+ Predicate pred = (isSigned ? ICmpInst::ICMP_SLT :
ICmpInst::ICMP_ULT);
+ return new ICmpInst(pred, V, Hi);
}
Likewise with the third definition of 'pred'.
@@ -3022,11 +3094,11 @@ Instruction *InstCombiner::visitAnd(Bina
uint64_t AndRHSMask = AndRHS->getZExtValue();
uint64_t TypeMask = Op0->getType()->getIntegralTypeMask();
uint64_t NotAndRHS = AndRHSMask^TypeMask;
// Optimize a variety of ((val OP C1) & C2) combinations...
- if (isa<BinaryOperator>(Op0) || isa<ShiftInst>(Op0)) {
+ if (isa<BinaryOperator>(Op0) || isa<ShiftInst>(Op0) ||
isa<CmpInst>(Op0)) {
The switch inside this 'if' doesn't handle compares, drop the extra isa.
@@ -4129,88 +4240,235 @@ Instruction *InstCombiner::visitSetCondI
...
+ // Test to see if the operands of the setcc are casted versions of
other
+ // values. If the cast can be stripped off both arguments, we do
so now.
+ if (CastInst *CI = dyn_cast<CastInst>(Op0)) {
+ Value *CastOp0 = CI->getOperand(0);
+ if (CI->isLosslessCast() && I.isEquality() &&
+ (isa<Constant>(Op1) || isa<CastInst>(Op1))) {
This transformation doesn't apply to FP icmp, because there are no
lossless FP casts.
+ if (I.isEquality()) {
+ Value *A, *B;
+ if (match(Op0, m_Sub(m_Value(A), m_Value(B))) && A == Op1) {
+ // (A-B) == A -> B == 0
+ return BinaryOperator::create(I.getOpcode(), B,
+ Constant::getNullValue(B->getType
()));
+ } else if (match(Op1, m_Sub(m_Value(A), m_Value(B))) && A == Op0) {
+ // A == (A-B) -> B == 0
+ return BinaryOperator::create(I.getOpcode(), B,
+ Constant::getNullValue(B->getType
()));
+ }
+ }
This xform is unsafe for FP, don't do it.
+ // icmp's with boolean values can always be turned into bitwise
operations
if (Ty == Type::BoolTy) {
This code is not correct for signed comparisons of bools. Rememember
that
true <s false --> true, but
true <u false --> false
@@ -4552,15 +4801,12 @@ Instruction *InstCombiner::visitSetCondI
// vice versa). This is because (x /s C1) <s C2 produces
different
// results than (x /s C1) <u C2 or (x /u C1) <s C2 or even
// (x /u C1) <u C2. Simply casting the operands and
result won't
// work. :( The if statement below tests that condition
and bails
// if it finds it.
- const Type *DivRHSTy = DivRHS->getType();
- unsigned DivOpCode = LHSI->getOpcode();
- if (I.isEquality() &&
- ((DivOpCode == Instruction::SDiv && DivRHSTy-
>isUnsigned()) ||
- (DivOpCode == Instruction::UDiv && DivRHSTy->isSigned
())))
+ bool DivIsSigned = LHSI->getOpcode() == Instruction::SDiv;
+ if (I.isEquality() && DivIsSigned != DivRHS->getType()-
>isSigned())
break;
This was wrong before your patch. It should be something like:
if (!I.isEquality() && DivIsSigned != isSignedComparison
(I.getPredicate()))
break;
- // Replace (and X, (1 << size(X)-1) != 0) with x < 0,
converting X
- // to be a signed value as appropriate.
+ // Replace (and X, (1 << size(X)-1) != 0) with x < 0
Please make the comment read 'x <s 0' to be clear.
// ((X & ~7) == 0) --> X < 8
if (CI->isNullValue() && isHighOnes(BOC)) {
Value *X = BO->getOperand(0);
Constant *NegX = ConstantExpr::getNeg(BOC);
+ // If 'NegX' is signed, insert a cast now.
if (NegX->getType()->isSigned()) {
const Type *DestTy = NegX->getType()-
>getUnsignedVersion();
X = InsertCastBefore(Instruction::BitCast, X,
DestTy, I);
NegX = ConstantExpr::getBitCast(NegX, DestTy);
}
This cast can be removed.
+ // If this is a signed comparison, check for comparisons
in the
+ // vicinity of zero.
+ switch (I.getPredicate()) {
+ default: break;
+ case ICmpInst::ICMP_SLT: // X < 0 => x > 127
+ if (CI->isNullValue())
+ return new ICmpInst(ICmpInst::ICMP_UGT, CastOp,
+ ConstantInt::get(SrcTy, (1ULL <<
(SrcTySize-1))-1));
+ break;
+ case ICmpInst::ICMP_SGT: // X > -1 => x < 128
+ if (cast<ConstantInt>(CI)->getSExtValue() == -1)
+ return new ICmpInst(ICmpInst::ICMP_ULT, CastOp,
+ ConstantInt::get(SrcTy, 1ULL <<
(SrcTySize-1)));
+ break;
+ case ICmpInst::ICMP_ULT: { // X < 128 => X > -1
+ ConstantInt *CUI = cast<ConstantInt>(CI);
+ if (CUI->getZExtValue() == 1ULL << (SrcTySize-1))
+ return new ICmpInst(ICmpInst::ICMP_SGT, CastOp,
+ ConstantInt::get(SrcTy, -1));
+ break;
+ }
+ case ICmpInst::ICMP_UGT: { // X > 127 => X < 0
+ ConstantInt *CUI = cast<ConstantInt>(CI);
+ if (CUI->getZExtValue() == (1ULL << (SrcTySize-1))-1)
+ return new ICmpInst(ICmpInst::ICMP_SLT, CastOp,
+ Constant::getNullValue(SrcTy));
+ break;
+ }
These transformations are no longer needed, please remove: "X < 0 =>
x > 127" and "X > -1 => x < 128". The two we keep are useful to
canonicalize comparisons against constants to use smaller constants.
In their place, please add a transformation that changes:
icmp pred (bitcast X), cst
to:
icmp pred X, (bitcast cst)
Make sure to check that X is integral, to prevent fp<->int bitcasts.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061222/e3d56192/attachment.html>
More information about the llvm-commits
mailing list