[llvm-commits] SETCC Patches (For Review Only) #1: llvm-gcc

[Chris Lattner]

On Dec 10, 2006, at 3:43 PM, Reid Spencer wrote:
> Here's the SETCC patch to convert SetCondInst (SetCC) instructions  
> into
> ICmpInst. Here's somethings you should know about this patch:
>
> 3. The SetCondInst instruction is still used for floating point
> comparisons.

Ok, this will be changed in the next patch?

> 6. The llvm-gcc patch emits ICmp for integer/pointer and SetCC for
> floating
>    point. No FCmpInst instructions are emitted. We'll do that in the
> next patch.

Ok, sounds great.

For the llvm-gcc patch:

    Value *EmitCompare(tree_node *exp, unsigned Opc, bool isUnordered);
+  Value *EmitCompare(tree_node *exp, unsigned Opc);

I don't like the subtle overloads here.  One way to fix this:

+  case LT_EXPR: {
+    tree Op0Ty = TREE_TYPE(TREE_OPERAND(exp,0));
+    if (!FLOAT_TYPE_P(Op0Ty))
+      Result =
+        EmitCompare(exp, TYPE_UNSIGNED(Op0Ty) ?
+                    ICmpInst::ICMP_ULT : ICmpInst::ICMP_SLT);
+    else
+      Result = EmitCompare(exp, Instruction::SetLT, 0);
+    break;
+  }
+  case LE_EXPR: {
+    tree Op0Ty = TREE_TYPE(TREE_OPERAND(exp,0));
+    if (!FLOAT_TYPE_P(Op0Ty))
+      Result =
+        EmitCompare(exp, TYPE_UNSIGNED(Op0Ty) ?
+                    ICmpInst::ICMP_ULE : ICmpInst::ICMP_SLE);
+    else
+      Result = EmitCompare(exp, Instruction::SetLE, 0);
+    break;
+  }

This logic shouldn't be duplicated everywhere.  I much prefer that  
you do something like:

case LT_EXPR:
   Result = EmitCompare(exp, ICmpInst::ICMP_ULT, ICmpInst::ICMP_SLT,  
Instruction::SetLT, 0);
   break;

... and move the code for determining which opcode to use into  
EmitCompare.  The same can be
done for EmitMinMaxExpr.


@@ -2261,8 +2319,8 @@
    Value *Op = Emit(TREE_OPERAND(exp, 0), 0);
    if (!Op->getType()->isFloatingPoint()) {
      Instruction *OpN = BinaryOperator::createNeg(Op, Op->getName() 
+"neg",CurBB);
-    Value *Cmp = new SetCondInst(Instruction::SetGE, Op, OpN- 
 >getOperand(0),
-                                 "abscond", CurBB);
+    Value *Cmp = new ICmpInst(ICmpInst::ICMP_SGE, Op, OpN->getOperand 
(0),
+                              "abscond", CurBB);
      return new SelectInst(Cmp, Op, OpN, "abs", CurBB);
    } else {
      // Turn FP abs into fabs/fabsf.

This isn't right.  You need to emit a signed or unsigned comparison  
depending on TYPE_UNSIGNED.  It would make sense to just use a call  
to EmitCompare here and have it pick the right one.  I know that  
unsigned abs doesn't make much sense, but this is how expr.c handles  
it for GCC:

       /* Unsigned abs is simply the operand.  Testing here means we  
don't
          risk generating incorrect code below.  */
       if (TYPE_UNSIGNED (type))
         return op0;

@@ -2464,8 +2542,13 @@
    LHS = NOOPCastToType(LHS, Ty);
    RHS = NOOPCastToType(RHS, Ty);

-  Value *Pred = new SetCondInst((Instruction::BinaryOps)CmpOpc, LHS,  
RHS,
-                                "tmp", CurBB);
+  Value *Pred;
+  if (LHS->getType()->isFloatingPoint())
+    Pred = new SetCondInst((Instruction::BinaryOps)CmpOpc, LHS, RHS,
+                           "tmp", CurBB);
+  else
+    Pred = new ICmpInst((ICmpInst::Predicate)CmpOpc, LHS, RHS,
+                             "tmp", CurBB);
    return new SelectInst(Pred, LHS, RHS,
                          TREE_CODE(exp) == MAX_EXPR ? "max" : "min",  
CurBB);
  }

Likewise this code is wrong (for max).  It should also just call  
EmitCompare.

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061210/8e3c7013/attachment.html>