[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