[llvm-commits] SETCC Patches #2

[Reid Spencer]

Hi Chris,

Some feedback on your review. Please note the attachments which also
need to be reviewed. These are for changes to ConstantFolding and
ConstantRange.

Thanks,

Reid.

On Sun, 2006-12-10 at 19:05 -0800, Chris Lattner wrote: 
> 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.

Yes, good idea. 
> 
> 
> Also, perhaps I just didn't see it, but it seems that the SetCondInst
> ctor should assert that the operands are FP types now.

Yes, I added that after I made/sent the patch. It only tripped on a few
llvm/test casts that had not run llvm-upgrade. I fixed those test
cases. 


> 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 :)

fp vectors -> setcondinst
int vectors -> icmpinst

I haven't really checked for this much. Taken as a to-do item. 

> 
> 
> 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.

Fixed.

> 
> 
> 
> 
> +<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.  

Fixed.

> Please ensure this validates.  

Yes, after commit.

> Why not mention CastInst, TerminatorInst, etc here?

Okay.

> 
> 
> 
> 
> 
> 
> ===================================================================
> 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'.

Fixed.

> 
> 
> 
> 
> +  /// 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.

Fixed.

> 
> 
> --- 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. 

Okay, good idea. Could you please review the CF.patch file which is
attached. I think this is what you had in mind.

> Further, 'lessthan' drops the sign, so the code is apparently wrong.

I'm not following this. I thought you weren't allowed to do relational
operators on packed type? Are you talking about the ConstantInt case?
This:
  static Constant *LessThan(const ConstantInt *V1, const ConstantInt
*V2) {
    bool R = (BuiltinType)V1->getZExtValue() <
(BuiltinType)V2->getZExtValue();
    return ConstantBool::get(R);
  }


> 
> 
> +/// 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 :)

Fixed. 
> 
> 
> 
> 
> +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.

The LessThan code above for ConstantInt works fine. It extracts the
canonical ZExt form then casts each operand to the correct type before
doing the compare:
   bool R = (BuiltinType)V1->getZExtValue() <
(BuiltinType)V2->getZExtValue();

I don't see the problem. 
> 
> 
> 
> 
> 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 :)

Good idea. Done. 
> 
> 
> 
> 
> 
> 
> Instruction::isComparison should probably go away completely when
> you're done (when the fp part lands), or at least be inlined in the
> header.

Yup. Taken as a to-do. 
> 
> 
> 
> 
> ===================================================================
> 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 ==/!=.

Done.

> 
> 
> ===================================================================
> 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.

Yeah, I didn't get a chance yet, but I just made it signless and made
all its callers
pass the signedness down. Fortunately, there weren't many of them.

> 
> 
> 
> 
> @@ -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.

In the fix to ConstantRange, getMinValue doesn't take an "isSigned"
argument. Its always using unsigned. This is allowed because the
ConstantInt values used to construct it are
also signless. I've attached the diff to ConstantRange.cpp for review in
ConstRange.patch.

> 
> --- 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.
> 
Nice. Deleted.

> 
> 
> 
> 
> 
> ===================================================================
> 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.
> 
Right. Deleted.
> 
> 
> +    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.

Removed.

> 
> 
> 
> 
> 
> 
> @@ -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.

Yes, its an implementation method internal to the file and there is only
one call. The predicate value passed in is derived from
ICmpInst::getPredicate() so I dumped the entire if statement and just:

    Result = ConstantExpr::getICmp(SetCCOpcode, Result, RHS);

> 
> 
> 
> 
> @@ -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.

Yup. Done.

> 
> 
> 
> 
> @@ -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.

Yup. Done previously. SetCondInst does not now appear in
ScalarEvolution.cpp


> -Chris
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CF.patch
Type: text/x-patch
Size: 27647 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061213/3d575b3f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ConstRange.patch
Type: text/x-patch
Size: 14581 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061213/3d575b3f/attachment-0001.bin>