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

[Reid Spencer]

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?

Yes. This was done to minimize patch size, although I'm not sure it
helped that much.

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

That's really gross. One or two arguments will always be ignored
depending on whether it is a floating point or an integer compare. If
your objection is the overloading, how about EmitFPCompare and
EmitIntCompare?  If you want to shorten the logic in the switch cases we
could write an inline name EmitCompare to do what you suggested.

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

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

Okay.
> 
> 
> -Chris