[llvm-commits] SETCC InstructionCombining.cpp [3/3]
Chris Lattner
clattner at apple.com
Fri Dec 22 13:31:33 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.
This was the easiest of the three. woo!
-Chris
- // Handle the special case of: setcc (cast bool to X), <cst>
+ // Handle the special case of: icmp (cast bool to X), <cst>
// This comes up when you have code like
// int X = A < B;
// if (X) ...
// For generality, we handle any zero-extension of any operand
comparison
// with a constant or another cast from the same type.
if (isa<ConstantInt>(Op1) || isa<CastInst>(Op1))
- if (Instruction *R = visitSetCondInstWithCastAndCast(I))
+ if (Instruction *R = visitICmpInstWithCastAndCast(I))
return R;
}
Should this be checking for zext/sext(Op1) instead of castinst(op1) ?
+Instruction *InstCombiner::visitICmpInstWithCastAndCast(ICmpInst
&ICI) {
...
- if (CastInst *CI = dyn_cast<CastInst>(SCI.getOperand(1))) {
+ if (CastInst *CI = dyn_cast<CastInst>(ICI.getOperand(1))) {
// Not an extension from the same type?
RHSCIOp = CI->getOperand(0);
- if (RHSCIOp->getType() != LHSCIOp->getType()) return 0;
- } else if (ConstantInt *CI = dyn_cast<ConstantInt>(SCI.getOperand
(1))) {
This should presumably only allow zext or sext, not any cast.
+ if (Res2 == CI) {
+ // Make sure that sign of the Cmp and the sign of the Cast are
the same.
+ // For example, we might have:
+ // %A = sext short %X to uint
+ // %B = icmp ugt uint %A, 1330
+ // It is incorrect to transform this into
+ // %B = icmp ugt short %X, 1330
+ // because %A may have negative value.
+ //
+ // However, it is OK if SrcTy is bool (See cast-set.ll testcase)
+ // OR operation is EQ/NE.
+ if (isSignedExt == isSignedCmp || SrcTy == Type::BoolTy ||
ICI.isEquality())
+ return new ICmpInst(ICI.getPredicate(), LHSCIOp, Res1);
+ else
+ return 0;
+ }
Can you explain why srcty == bool is a special case?
+ // First, handle some easy cases. We no the result cannot be equal
at this
+ // point so handle the ICI.isEquality() cases
we no -> we know?
+ // We're performing a signed comparison.
+ if (isSignedExt) {
+ // Signed extend and signed comparison.
+ if (cast<ConstantInt>(CI)->getSExtValue() < 0)// X < (small) --
> false
+ Result = ConstantBool::getFalse();
+ else
+ Result = ConstantBool::getTrue(); // X < (large) --
> true
+ } else {
+ // Zero extend and signed comparison.
+ if (cast<ConstantInt>(CI)->getSExtValue() < 0)
+ Result = ConstantBool::getFalse();
+ else
+ Result = ConstantBool::getTrue();
}
My reading of this code says that you can eliminate the 'if
issignedext' test, as the if/then cases are the same. Is this a bug
or just code duplication?
@@ -5933,13 +6195,12 @@ Instruction *InstCombiner::commonIntCast
return new ShiftInst(Instruction::LShr, Op0, Op1);
}
}
break;
- case Instruction::SetEQ:
- case Instruction::SetNE:
- // If we are just checking for a seteq of a single bit and
casting it
+ case Instruction::ICmp:
+ // If we are just checking for a icmp_eq of a single bit and
casting it
// to an integer. If so, shift the bit to the appropriate
place then
This code needs to accept and correctly ignore the icmp cases that
aren't ==/!=. Currently anything not != is treated as == due to:
if (isPowerOf2_64(KnownZero^TypeMask)) { // Exactly 1
possible 1?
- bool isSetNE = SrcI->getOpcode() == Instruction::SetNE;
+ bool isNE =
+ cast<ICmpInst>(SrcI)->getPredicate() == ICmpInst::ICMP_NE;
@@ -6313,10 +6581,17 @@ Instruction *InstCombiner::FoldSelectOpO
if (BinaryOperator *BO = dyn_cast<BinaryOperator>(TI)) {
if (MatchIsOpZero)
return BinaryOperator::create(BO->getOpcode(), MatchOp, NewSI);
else
return BinaryOperator::create(BO->getOpcode(), NewSI, MatchOp);
+ } else if (CmpInst *CI = dyn_cast<CmpInst>(TI)) {
+ if (MatchIsOpZero)
+ return CmpInst::create(CI->getOpcode(), CI->getPredicate(),
+ MatchOp, NewSI);
+ else
+ return CmpInst::create(CI->getOpcode(), CI->getPredicate(),
+ NewSI, MatchOp);
This doesn't seem right to me, you can't just swap operands like
this. Is FoldSelectOpOp even called for compares? If not, plz
remove this code and the other changes you made.
+ // (x <s 0) ? -1 : 0 -> ashr x, 31
+ // (x >u 2147483647) ? -1 : 0 -> ashr x, 31
if (TrueValC->isAllOnesValue() && FalseValC->isNullValue())
if (ConstantInt *CmpCst = dyn_cast<ConstantInt>(IC-
>getOperand(1))) {
bool CanXForm = false;
- if (CmpCst->getType()->isSigned())
+ if (IC->isSignedPredicate())
CanXForm = CmpCst->isNullValue() &&
- IC->getOpcode() == Instruction::SetLT;
+ IC->getPredicate() == ICmpInst::ICMP_SLT;
else {
unsigned Bits = CmpCst->getType()-
>getPrimitiveSizeInBits();
CanXForm = (CmpCst->getZExtValue() == ~0ULL >> (64-
Bits+1)) &&
- IC->getOpcode() == Instruction::SetGT;
+ IC->getPredicate() == ICmpInst::ICMP_UGT;
}
If other parts of instcombine canonicalize (x >u 2147483647) -> (x <s
0), you can simplify this by not handling the (x >u 2147483647) case
here.
@@ -6550,10 +6847,12 @@ Instruction *InstCombiner::visitSelectIn
new SelectInst(SI.getCondition(), TVI->getOperand(2-
OpToFold), C,
Name);
InsertNewInstBefore(NewSel, SI);
if (BinaryOperator *BO = dyn_cast<BinaryOperator>(TVI))
return BinaryOperator::create(BO->getOpcode(),
FalseVal, NewSel);
+ else if (ICmpInst *ICI = dyn_cast<ICmpInst>(TVI))
+ return new ICmpInst(ICI->getPredicate(), FalseVal,
NewSel);
This code is dead, please remove.
@@ -6578,10 +6877,12 @@ Instruction *InstCombiner::visitSelectIn
new SelectInst(SI.getCondition(), C, FVI->getOperand
(2-OpToFold),
Name);
InsertNewInstBefore(NewSel, SI);
if (BinaryOperator *BO = dyn_cast<BinaryOperator>(FVI))
return BinaryOperator::create(BO->getOpcode(),
TrueVal, NewSel);
+ else if (ICmpInst *ICI = dyn_cast<ICmpInst>(FVI))
+ return new ICmpInst(ICI->getPredicate(), TrueVal,
NewSel);
Likewise.
@@ -6985,11 +7286,11 @@ bool InstCombiner::transformConstExprCas
const Type *ActTy = (*AI)->getType();
ConstantInt *c = dyn_cast<ConstantInt>(*AI);
//Either we can cast directly, or we can upconvert the argument
bool isConvertible = ActTy->canLosslesslyBitCastTo(ParamTy) ||
(ParamTy->isIntegral() && ActTy->isIntegral() &&
- ParamTy->isSigned() == ActTy->isSigned() &&
+// ParamTy->isSigned() == ActTy->isSigned() &&
ParamTy->getPrimitiveSize() >= ActTy->getPrimitiveSize()) ||
(c && ParamTy->getPrimitiveSize() >= ActTy->getPrimitiveSize
() &&
c->getSExtValue() > 0);
if (Callee->isExternal() && !isConvertible) return false;
}
This is still needed.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061222/c3e1ac5b/attachment.html>
More information about the llvm-commits
mailing list