[llvm-commits] SETCC Patch [For Review Only]

[Chris Lattner]

On Nov 18, 2006, at 11:36 PM, Reid Spencer wrote:

> This is the first in a series of patches to split the SETCC  
> instructions
> into just two instructions: icmp (integer compare) and fcmp (floating
> point compare). This patch is a no-op. It introduces code that will be
> used in subsequent patches. The new instruction codes are reserved,  
> the
> instruction classes are defined, and the InstVisitor class (and users)
> is updated to recognize the new instructions. However, nothing else in
> LLVM will produce these instructions (yet). The patch passes all  
> tests.

The comments on fcmpinst are not correct in some cases:
+  /// For example, EQ -> NE, UGT -> ULE, SLT -> SGE, etc.
+  /// @returns the inverse predicate for the instructions current  
predicate.
+  /// @brief Return the inverse of the predicate

These refer to SLT which isn't valid for fcmp, similarly for others.

--- include/llvm/Support/InstVisitor.h	9 Oct 2006 18:33:08 -0000	1.41
+++ include/llvm/Support/InstVisitor.h	19 Nov 2006 07:32:51 -0000
@@ -167,10 +167,12 @@ public:
    RetTy visitSwitchInst(SwitchInst &I)              { DELEGATE 
(TerminatorInst);}
    RetTy visitInvokeInst(InvokeInst &I)              { DELEGATE 
(TerminatorInst);}
    RetTy visitUnwindInst(UnwindInst &I)              { DELEGATE 
(TerminatorInst);}
    RetTy visitUnreachableInst(UnreachableInst &I)    { DELEGATE 
(TerminatorInst);}
    RetTy visitSetCondInst(SetCondInst &I)            { DELEGATE 
(BinaryOperator);}
+  RetTy visitICmpInst(ICmpInst &I)                  { DELEGATE 
(Instruction);}
+  RetTy visitFCmpInst(FCmpInst &I)                  { DELEGATE 
(Instruction);}
    RetTy visitMallocInst(MallocInst &I)              { DELEGATE 
(AllocationInst);}
    RetTy visitAllocaInst(AllocaInst &I)              { DELEGATE 
(AllocationInst);}
    RetTy visitFreeInst(FreeInst     &I)              { DELEGATE 
(Instruction); }
    RetTy visitLoadInst(LoadInst     &I)              { DELEGATE 
(Instruction); }
    RetTy visitStoreInst(StoreInst   &I)              { DELEGATE 
(Instruction); }

This should delegate to CmpInst, which delegates to Instruction.


+void CmpInst::swapOperands() {
+  if (ICmpInst* IC = dyn_cast<ICmpInst>(this)) {
+    IC->swapOperands();
+  } else if (FCmpInst* FC = dyn_cast<FCmpInst>(this)) {
+    FC->swapOperands();
+  }
+  assert(!"Unknown CmpInst type");
+}

I'd write this as:

+void CmpInst::swapOperands() {
+  if (ICmpInst* IC = dyn_cast<ICmpInst>(this)) {
+    IC->swapOperands();
+  } else {
+    cast<FCmpInst>(this)->swapOperands();
+  }

which is faster in release builds.  Besides, the assert will always  
trigger above.  Please
use 0 && "foo", which is the standard idiom instead of !"foo".


+FCmpInst::Predicate FCmpInst::getInversePredicate(Predicate pred) {
+  switch (pred) {
+    default:
+      assert(!"Unknown icmp predicate!");
+    case OEQ: return ONE;
+    case ONE: return OEQ;
+    case OGT: return OLE;
+    case OLT: return OGE;
+    case OGE: return OLT;
+    case OLE: return OGT;
+    case UEQ: return UNE;
+    case UNE: return UEQ;
+    case UGT: return ULE;
+    case ULT: return UGE;
+    case UGE: return ULT;
+    case ULE: return UGT;
+    case TRUE: return FALSE;
+    case FALSE: return TRUE;
+  }
+}

These aren't right, for example, !OEQ = UNE


+void Verifier::visitICmpInst(ICmpInst& IC) {
+  // Check that icmp instructions return bool
+  Assert1(IC.getType() == Type::BoolTy,
+         "ICmp instructions must return boolean values!", &IC);
+  // Check that the operands are the same type
+  const Type* Op0Ty = IC.getOperand(0)->getType();
+  const Type* Op1Ty = IC.getOperand(0)->getType();
+  Assert1(Op0Ty == Op1Ty,
+          "Both operands to ICmp instruction are not of the same  
type!", &IC);
+  // Check that the operands are the right type
+  Assert1(Op0Ty->isIntegral() || Op0Ty->getTypeID() ==  
Type::PointerTyID ||
+          (isa<PackedType>(Op0Ty) &&
+           cast<PackedType>(Op0Ty)->getElementType()->isIntegral()),
+          "Invalid operand types for ICmp instruction", &IC);

Most of these check should also happen in the ctors.  The check for  
the result of bool should just be in the ctor.  Once an instruction  
is created, you can't change its result type.


+void  BVNImpl::visitCmpInst(CmpInst &CI1) {
+  Value *LHS = CI1.getOperand(0);
+  for (Value::use_iterator UI = LHS->use_begin(), UE = LHS->use_end();
+       UI != UE; ++UI)
+    if (CmpInst *CI2 = dyn_cast<CmpInst>(*UI))
+      // Check to see if this comparinstruction is not CI, but same  
opcode,
+      // same predicate, and in the same function.

typo comparinstruction

+            // And its commutative (equal or not equal)
+             ((isa<ICmpInst>(CI1) && cast<ICmpInst> 
(CI1).isCommutative()) ||
+              (isa<FCmpInst>(CI1) && cast<ICmpInst> 
(CI1).isCommutative()))))

The second line has a typo (ICmp -> FCmp).  The second line begs for  
a CmpInst::isCommutative()

Go ahead and commit after tweaking these,

-Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061119/d483377f/attachment.html>