[llvm-commits] SETCC InstructionCombining.cpp [#1/n]

[Chris Lattner]

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.

+// SimplifyCompare - For a CmpInst this function just orders the  
operands
+// so that theyare listed from right (least complex) to left (most  
complex).
+// This puts constants before unary operators before binary operators.
+//
+bool InstCombiner::SimplifyCompare(CmpInst &I) {

Please doxygenify the comment.

+  bool Changed = false;
+  if (getComplexity(I.getOperand(0)) < getComplexity(I.getOperand 
(1))) {
+    I.swapOperands();
+    Changed = true;
+  }
+  // Compare instructions are not associative so there's nothing  
else we can do.
+  return Changed;
+}

Why not:

   if (getComplexity(I.getOperand(0)) >= getComplexity(I.getOperand(1)))
     return false;
   I.swapOperands();
   return true;

?



  // isTrueWhenEqual - Return true if the specified setcondinst  
instruction is
  // true when both operands are equal...
  //
  static bool isTrueWhenEqual(Instruction &I) {
+  if (ICmpInst *ICI = dyn_cast<ICmpInst>(&I))
+    return ICI->getPredicate() == ICmpInst::ICMP_EQ  ||
+           ICI->getPredicate() == ICmpInst::ICMP_UGE ||
+           ICI->getPredicate() == ICmpInst::ICMP_SGE ||
+           ICI->getPredicate() == ICmpInst::ICMP_ULE ||
+           ICI->getPredicate() == ICmpInst::ICMP_SLE;
    return I.getOpcode() == Instruction::SetEQ ||
           I.getOpcode() == Instruction::SetGE ||
           I.getOpcode() == Instruction::SetLE;
  }

This should be split into two functions, one for ICmpInst and one for  
FCmpInst/SetCondInst.  Since your touching it, plz doxygenify also. :)



@@ -1580,21 +1607,31 @@ static Value *FoldOperationIntoSelectOpe
    // Figure out if the constant is the left or the right argument.
    bool ConstIsRHS = isa<Constant>(I.getOperand(1));
    Constant *ConstOperand = cast<Constant>(I.getOperand(ConstIsRHS));

    if (Constant *SOC = dyn_cast<Constant>(SO)) {
-    if (ConstIsRHS)
-      return ConstantExpr::get(I.getOpcode(), SOC, ConstOperand);
-    return ConstantExpr::get(I.getOpcode(), ConstOperand, SOC);
+    if (CmpInst *CI = dyn_cast<CmpInst>(&I)) {
+      unsigned short pred = CI->getPredicate();
+      if (ConstIsRHS)
+        return ConstantExpr::getCompare(pred, SOC, ConstOperand);
+      return ConstantExpr::getCompare(pred, ConstOperand, SOC);

This code doesn't appear to be called for compares.  Is it?  If so,  
the code is broken, you can't just swap the LHS/RHS of a compare like  
that without breaking things.


+/// isSignBitCheck - Given an exploded icmp instruction, return true  
if it
  /// really just returns true if the most significant (sign) bit is  
set.
+static bool isSignBitCheck(ICmpInst::Predicate pred, Value *LHS,
+                           ConstantInt *RHS) {

There is no reason to pass LHS into this function (this is an  
absolute statement, not a bug in your patch) please remove LHS.


+    case ICmpInst::ICMP_UGE:
+      // True if LHS u>= RHS and RHS == high-bit-mask (2^7, 2^15,  
2^31, etc)
+      return RHS->getZExtValue() == 1ULL <<
+        RHS->getType()->getPrimitiveSizeInBits()-1;

Please add parens to make the precedence more explicit, even though  
it is right.


More tomorrow,

-Chris