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

[Reid Spencer]

Chris,

I rewrote the llvm-gcc portion of the SETCC patch with your suggestions.
It is much cleaner now. The patch is attached. Hopefully this is more to
your liking.

Reid.

Sheng: This is just FYI.

On Sun, 2006-12-10 at 18:03 -0800, Chris Lattner wrote:
> 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 --------------
A non-text attachment was scrubbed...
Name: SETCC-LLVMGCC.patch
Type: text/x-patch
Size: 9347 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061213/9f148a36/attachment.bin>