[llvm-commits] SETCC Patches (For Review Only) #1: llvm-gcc
Chris Lattner
clattner at apple.com
Sun Dec 10 18:03:55 PST 2006
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>
More information about the llvm-commits
mailing list