[llvm-commits] SETCC Patches (For Review Only) #1: llvm-gcc
Reid Spencer
rspencer at reidspencer.com
Wed Dec 13 11:21:07 PST 2006
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>
More information about the llvm-commits
mailing list