[llvm-commits] SETCC Patches #2
Chris Lattner
clattner at apple.com
Sun Dec 10 19:05:12 PST 2006
Like for div/rem, I strongly recommend a patch (e.g. to instcombine)
that will result in signed comparisons with unsigned operands or
signed operands to unsigned comparisons. Without that, you are not
testing a bunch of hard cases.
Also, perhaps I just didn't see it, but it seems that the SetCondInst
ctor should assert that the operands are FP types now.
Another question: currently do you handle integer vectors with icmp
and fp vectors with setcondinst? You should verify, as the testsuite
doesn't have good coverage of vectors, but you'll get cranky emails
from me if they break :)
I'm almost half done reviewing the patch.
Comments below,
--- docs/ProgrammersManual.html 7 Dec 2006 20:04:41 -0000 1.96
+++ docs/ProgrammersManual.html 10 Dec 2006 23:26:33 -0000
..
<!--
_______________________________________________________________________
-->
<div class="doc_subsubsection">
+ <a name="m_Instruction">Important Subclasses of the
<tt>Instruction</tt>
+ class</a>
+</div>
The m_Instruction anchor is already used, pick another one.
+<div class="doc_text">
+ <ul>
+ <li><tt><a name="BinaryOperator">BinaryOperator</a></tt>
+ <p>This subclasses represents all two operand instructions whose
operands
+ must be the same type, except for the comparison instructions.</
p></li>
+ <li><tt><a name="CmpInst">CmpInst</a></tt>
+ <p>This subclasses respresents the two comparison instructions,
+ <a href="LangRef.html#i_icmp">ICmpInst</a> (integer opreands), and
+ <a href="LangRef.html#i_fcmp">FCmpInst</a> (floating point
operands).</p>
+ </ul>
+ </dif>
dif -> div. Please ensure this validates. Why not mention CastInst,
TerminatorInst, etc here?
===================================================================
RCS file: /var/cvs/llvm/llvm/include/llvm/Instruction.h,v
retrieving revision 1.75
diff -t -d -u -p -5 -r1.75 Instruction.h
--- include/llvm/Instruction.h 27 Nov 2006 01:05:09 -0000 1.75
+++ include/llvm/Instruction.h 10 Dec 2006 23:26:33 -0000
@@ -69,10 +69,20 @@ public:
+ /// This function determines if the specified instruction executes
the same
+ /// operation as the current one. This means that the opcodes,
type, operand
+ /// type and any other factors affecting the operation must be the
same. This
+ /// is similar to isIdenticalTo excep the operands themselves
don't have to
+ /// be identical.
typo 'excep'.
+ /// This is a static version that you can use without an instruction.
+ /// @brief Return the predicate for a signed operand.
+ static Predicate getSignedPredicate(Predicate pred);
"for a signed comparison.". The sign of the operand doesn't matter.
--- lib/VMCore/ConstantFolding.cpp 6 Dec 2006 00:25:09 -0000 1.110
+++ lib/VMCore/ConstantFolding.cpp 10 Dec 2006 23:26:35 -0000
@@ -427,13 +427,18 @@ struct VISIBILITY_HIDDEN ConstantPackedR
static Constant *LessThan(const ConstantPacked *V1, const
ConstantPacked *V2){
return 0;
}
static Constant *EqualTo(const ConstantPacked *V1, const
ConstantPacked *V2) {
for (unsigned i = 0, e = V1->getNumOperands(); i != e; ++i) {
- Constant *C =
- ConstantExpr::getSetEQ(const_cast<Constant*>(V1->getOperand
(i)),
- const_cast<Constant*>(V2->getOperand
(i)));
+ Constant *C;
+ if (V1->getOperand(i)->getType()->isFloatingPoint())
+ C = ConstantExpr::getSetEQ(const_cast<Constant*>(V1-
>getOperand(i)),
+ const_cast<Constant*>(V2-
>getOperand(i)));
+ else
+ C = ConstantExpr::getICmp(ICmpInst::ICMP_EQ,
+ const_cast<Constant*>(V1-
>getOperand(i)),
+ const_cast<Constant*>(V2-
>getOperand(i)));
if (ConstantBool *CB = dyn_cast<ConstantBool>(C))
return CB;
}
It would be better to split up the "EqualTo" predicate here so that
there was one for FP and one for integers. When the FP part of this
lands, you'll want "Unordered" "Equal" and "LessThan" instead of just
LessThan + Equal. Further, 'lessthan' drops the sign, so the code is
apparently wrong.
+/// evaluateICmpRelation - This function determines if there is
anything we can
+/// decide about the two constants provided. This doesn't need to
handle simple
+/// things like integer comparisons, but should instead handle
ConstantExprs
+/// and GlobalValuess. If we can determine that the two constants
have a
GlobalValuess -> typo :)
+Constant *llvm::ConstantFoldCompareInstruction(
+ unsigned opcode, Constant *C1, Constant *C2, unsigned short
predicate) {
...
+ case ICmpInst::ICMP_ULT:
+ case ICmpInst::ICMP_SLT: C = ConstRules::get(C1, C2).lessthan
(C1, C2);break;
+ case ICmpInst::ICMP_UGT:
+ case ICmpInst::ICMP_SGT: C = ConstRules::get(C1, C2).lessthan
(C2, C1);break;
+ case ICmpInst::ICMP_ULE:
+ case ICmpInst::ICMP_SLE: // C1 <= C2 === !(C2 < C1)
+ C = ConstRules::get(C1, C2).lessthan(C2, C1);
+ if (C) return ConstantExpr::getNot(C);
+ break;
+ case ICmpInst::ICMP_UGE:
+ case ICmpInst::ICMP_SGE: // C1 >= C2 === !(C1 < C2)
+ C = ConstRules::get(C1, C2).lessthan(C1, C2);
+ if (C) return ConstantExpr::getNot(C);
+ break;
These drop the sign of the comparison, which is a serious bug. I'd
expect this to miscompile things like:
%X = iset slt ubyte 0, ubyte 128
... or whatever the syntax is.
Index: lib/VMCore/Constants.cpp
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/VMCore/Constants.cpp,v
retrieving revision 1.184
diff -t -d -u -p -5 -r1.184 Constants.cpp
--- lib/VMCore/Constants.cpp 8 Dec 2006 18:06:15 -0000 1.184
+++ lib/VMCore/Constants.cpp 10 Dec 2006 23:26:36 -0000
...
+ case Instruction::ICmp:
+ case Instruction::FCmp:
+ New = ConstantExpr::getCompare(OldC->getOpcode(), OldC-
>getPredicate(),
+ OldC->getOperand(0),OldC-
>getOperand(1));
+ break;
Why does ConstantExpr::getCompare take both an opcode and predicate?
It seems that the predicate would be enough. Likewise I'd expect
ConstantFoldCompareInstruction to just take
the predicate. The opcode is doubly redundant: both the type of the
operands *and* the predicate tell you what the opcode should be :)
Instruction::isComparison should probably go away completely when
you're done (when the fp part lands), or at least be inlined in the
header.
===================================================================
RCS file: /var/cvs/llvm/llvm/include/llvm/InstrTypes.h,v
retrieving revision 1.53
diff -t -d -u -p -5 -r1.53 InstrTypes.h
--- include/llvm/InstrTypes.h 7 Dec 2006 04:18:31 -0000 1.53
+++ include/llvm/InstrTypes.h 10 Dec 2006 23:26:33 -0000
+ /// @brief Determine if the predicate is an unsigned operation.
+ static bool isUnsigned(unsigned short predicate);
+
+ /// @brief Determine if the predicate is an signed operation.
+ static bool isSigned(unsigned short predicate);
The comments should indicate that these return false for ==/!=.
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/Analysis/ConstantRange.cpp,v
retrieving revision 1.21
diff -t -d -u -p -5 -r1.21 ConstantRange.cpp
--- lib/Analysis/ConstantRange.cpp 7 Dec 2006 01:30:31 -0000 1.21
+++ lib/Analysis/ConstantRange.cpp 10 Dec 2006 23:26:37 -0000
static bool LT(ConstantIntegral *A, ConstantIntegral *B) {
- Constant *C = ConstantExpr::getSetLT(A, B);
+ Constant *C = ConstantExpr::getICmp(
+ A->getType()->isSigned() ? ICmpInst::ICMP_SLT :
ICmpInst::ICMP_ULT, A, B);
assert(isa<ConstantBool>(C) && "Constant folding of integrals not
impl??");
return cast<ConstantBool>(C)->getValue();
}
static bool LTE(ConstantIntegral *A, ConstantIntegral *B) {
- Constant *C = ConstantExpr::getSetLE(A, B);
+ Constant *C = ConstantExpr::getICmp(
+ A->getType()->isSigned() ? ICmpInst::ICMP_SLE :
ICmpInst::ICMP_ULE, A, B);
assert(isa<ConstantBool>(C) && "Constant folding of integrals not
impl??");
return cast<ConstantBool>(C)->getValue();
}
Unless I'm missing something, this looks dangerously wrong. The type
of the operands shouldn't determine the signedness. I thought you
were going to make ConstantRange be signless? If so, the queries
should pass down the sign they want.
@@ -168,17 +176,19 @@ ConstantRange::ConstantRange(unsigned Se
const Type *ConstantRange::getType() const { return Lower->getType
(); }
/// isFullSet - Return true if this set contains all of the
elements possible
/// for this data-type
bool ConstantRange::isFullSet() const {
- return Lower == Upper && Lower == getMaxValue(getType());
+ return Lower == Upper &&
+ Lower == getMaxValue(getType(), getType()->isSigned());
}
/// isEmptySet - Return true if this set contains no members.
///
bool ConstantRange::isEmptySet() const {
- return Lower == Upper && Lower == getMinValue(getType());
+ return Lower == Upper &&
+ Lower == getMinValue(getType(), getType()->isSigned());
}
likewise.
--- lib/Analysis/LoopInfo.cpp 7 Dec 2006 01:30:31 -0000 1.81
+++ lib/Analysis/LoopInfo.cpp 10 Dec 2006 23:26:38 -0000
@@ -534,29 +534,51 @@ Instruction *Loop::getCanonicalInduction
/// times the loop will be executed. Note that this means that the
backedge of
/// the loop executes N-1 times. If the trip-count cannot be
determined, this
/// returns null.
///
Value *Loop::getTripCount() const {
- // Canonical loops will end with a 'setne I, V', where I is the
incremented
+ // Canonical loops will end with a 'cmp ne I, V', where I is the
incremented
// canonical induction variable and V is the trip count of the loop.
Instruction *Inc = getCanonicalInductionVariableIncrement();
if (Inc == 0) return 0;
PHINode *IV = cast<PHINode>(Inc->getOperand(0));
BasicBlock *BackedgeBlock =
IV->getIncomingBlock(contains(IV->getIncomingBlock(1)));
if (BranchInst *BI = dyn_cast<BranchInst>(BackedgeBlock-
>getTerminator()))
- if (BI->isConditional())
- if (SetCondInst *SCI = dyn_cast<SetCondInst>(BI->getCondition()))
+ if (BI->isConditional()) {
+ if (ICmpInst *ICI = dyn_cast<ICmpInst>(BI->getCondition())) {
+ if (ICI->getOperand(0) == Inc)
+ if (BI->getSuccessor(0) == getHeader()) {
+ if (ICI->getPredicate() == ICmpInst::ICMP_NE)
+ return ICI->getOperand(1);
+ } else if (ICI->getPredicate() == ICmpInst::ICMP_EQ) {
+ return ICI->getOperand(1);
+ }
+ }
+ else if (FCmpInst *FCI = dyn_cast<FCmpInst>(BI->getCondition
())) {
+ if (FCI->getOperand(0) == Inc)
+ if (BI->getSuccessor(0) == getHeader()) {
+ if (FCI->getPredicate() == FCmpInst::FCMP_UNE ||
+ FCI->getPredicate() == FCmpInst::FCMP_ONE)
+ return FCI->getOperand(1);
+ } else if (FCI->getPredicate() == FCmpInst::FCMP_UEQ ||
+ FCI->getPredicate() == FCmpInst::FCMP_OEQ) {
+ return ICI->getOperand(1);
+ }
+ }
+ else if (SetCondInst *SCI = dyn_cast<SetCondInst>(BI-
>getCondition())) {
if (SCI->getOperand(0) == Inc)
if (BI->getSuccessor(0) == getHeader()) {
if (SCI->getOpcode() == Instruction::SetNE)
return SCI->getOperand(1);
} else if (SCI->getOpcode() == Instruction::SetEQ) {
return SCI->getOperand(1);
}
+ }
+ }
The "getCanonicalInductionVariableIncrement" method only returns non-
null in the non-fp case, so please remove all the FP handling code
from this method.
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/Analysis/ScalarEvolution.cpp,v
retrieving revision 1.67
diff -t -d -u -p -5 -r1.67 ScalarEvolution.cpp
--- lib/Analysis/ScalarEvolution.cpp 7 Dec 2006 01:30:31 -0000 1.67
+++ lib/Analysis/ScalarEvolution.cpp 10 Dec 2006 23:26:39 -0000
@@ -1456,121 +1456,252 @@ SCEVHandle ScalarEvolutionsImpl::Compute
// FIXME: we should be able to handle switch instructions (with a
single exit)
// FIXME: We should handle cast of int to bool as well
BranchInst *ExitBr = dyn_cast<BranchInst>(ExitingBlock-
>getTerminator());
if (ExitBr == 0) return UnknownValue;
assert(ExitBr->isConditional() && "If unconditional, it can't be
in loop!");
- SetCondInst *ExitCond = dyn_cast<SetCondInst>(ExitBr->getCondition
());
- if (ExitCond == 0) // Not a setcc
- return ComputeIterationCountExhaustively(L, ExitBr->getCondition(),
- ExitBr->getSuccessor(0) ==
ExitBlock);
+ if (SetCondInst *ExitCond = dyn_cast<SetCondInst>(ExitBr-
>getCondition())) {
Likewise, the SCEV code does not handle any FP compares.
+ case ICmpInst::ICMP_SLT:
+ if (LHS->getType()->isInteger()) {
+ SCEVHandle TC = HowManyLessThans(LHS, RHS, L);
+ if (!isa<SCEVCouldNotCompute>(TC)) return TC;
+ }
+ break;
+ case ICmpInst::ICMP_SGT:
+ if (LHS->getType()->isInteger()) {
+ SCEVHandle TC = HowManyLessThans(RHS, LHS, L);
+ if (!isa<SCEVCouldNotCompute>(TC)) return TC;
+ }
+ break;
The isInteger if's are unneeded.
@@ -1672,11 +1803,15 @@ ComputeLoadConstantCompareIterationCount
Constant *Result = GetAddressedElementFromGlobal(GV, Indexes);
if (Result == 0) break; // Cannot compute!
// Evaluate the condition for this iteration.
- Result = ConstantExpr::get(SetCCOpcode, Result, RHS);
+ if (SetCCOpcode >= ICmpInst::FIRST_ICMP_PREDICATE &&
+ SetCCOpcode <= ICmpInst::LAST_ICMP_PREDICATE)
+ Result = ConstantExpr::getICmp(SetCCOpcode, Result, RHS);
+ else
+ Result = ConstantExpr::get(SetCCOpcode, Result, RHS);
if (!isa<ConstantBool>(Result)) break; // Couldn't decide for
sure
if (cast<ConstantBool>(Result)->getValue() == false) {
I believe this can only be integer comparisons, but please check.
@@ -2182,11 +2324,13 @@ SCEVHandle ScalarEvolutionsImpl::HowFarT
// If the value is a constant, check to see if it is known to be
non-zero
// already. If so, the backedge will execute zero times.
if (SCEVConstant *C = dyn_cast<SCEVConstant>(V)) {
Constant *Zero = Constant::getNullValue(C->getValue()->getType());
- Constant *NonZero = ConstantExpr::getSetNE(C->getValue(), Zero);
+ Constant *NonZero = Zero->getType()->isFloatingPoint() ?
+ ConstantExpr::getSetNE(C->getValue(), Zero) :
+ ConstantExpr::getICmp(ICmpInst::ICMP_NE, C->getValue(), Zero);
if (NonZero == ConstantBool::getTrue())
return getSCEV(Zero);
return UnknownValue; // Otherwise it will loop infinitely.
}
SCEVConstant can only be for integers, drop the FP handling case.
@@ -2243,44 +2387,86 @@ HowManyLessThans(SCEV *LHS, SCEV *RHS, c
if (!LoopEntryPredicate) return UnknownValue;
}
// Now that we found a conditional branch that dominates the
loop, check to
// see if it is the comparison we are looking for.
- SetCondInst *SCI =dyn_cast<SetCondInst>(LoopEntryPredicate-
>getCondition());
- if (!SCI) return UnknownValue;
- Value *PreCondLHS = SCI->getOperand(0);
- Value *PreCondRHS = SCI->getOperand(1);
- Instruction::BinaryOps Cond;
- if (LoopEntryPredicate->getSuccessor(0) == PreheaderDest)
- Cond = SCI->getOpcode();
- else
- Cond = SCI->getInverseCondition();
+ if (SetCondInst *SCI = dyn_cast<SetCondInst>(
+ LoopEntryPredicate->getCondition())) {
+ Value *PreCondLHS = SCI->getOperand(0);
+ Value *PreCondRHS = SCI->getOperand(1);
+ Instruction::BinaryOps Cond;
+ if (LoopEntryPredicate->getSuccessor(0) == PreheaderDest)
+ Cond = SCI->getOpcode();
+ else
+ Cond = SCI->getInverseCondition();
likewise, only integer.
-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061210/583c9cff/attachment.html>
More information about the llvm-commits
mailing list