[llvm-commits] DIV -> U/S/FDiv Patch For Review (Followup)
Reid Spencer
rspencer at reidspencer.com
Tue Oct 24 21:33:17 PDT 2006
Chris,
Here's my InstCombine feedback ..
On Mon, 2006-10-23 at 22:52 -0700, Chris Lattner wrote:
>
> Index: lib/Transforms/Scalar/InstructionCombining.cpp
> ===================================================================
> RCS
> file: /var/cvs/llvm/llvm/lib/Transforms/Scalar/InstructionCombining.cpp,v
> retrieving revision 1.527
> diff -t -d -u -p -5 -r1.527 InstructionCombining.cpp
> --- lib/Transforms/Scalar/InstructionCombining.cpp 20 Oct 2006
> 18:20:21 -0000 1.527
> +++ lib/Transforms/Scalar/InstructionCombining.cpp 23 Oct 2006
> 02:43:34 -0000
> @@ -1973,15 +1979,15 @@ Instruction *InstCombiner::visitSub(Bina
> InsertNewInstBefore(BinaryOperator::createNot(OtherOp,
> "B.not"), I);
> return BinaryOperator::createAnd(Op0, NewNot);
> }
>
>
> // 0 - (X sdiv C) -> (X sdiv -C)
> - if (Op1I->getOpcode() == Instruction::Div)
> + if (Op1I->getOpcode() == Instruction::SDiv)
> if (ConstantInt *CSI = dyn_cast<ConstantInt>(Op0))
> if (CSI->getType()->isSigned() && CSI->isNullValue())
> if (Constant *DivRHS =
> dyn_cast<Constant>(Op1I->getOperand(1)))
> - return BinaryOperator::createDiv(Op1I->getOperand(0),
> + return BinaryOperator::createSDiv(Op1I->getOperand(0),
>
> ConstantExpr::getNeg(DivRHS));
>
>
> *** You should be able to drop the 'CSI->getType()->isSigned()' check.
Yup. Done.
>
>
>
>
> + // (X / C1) / C2 -> X / (C1*C2)
> if (Instruction *LHS = dyn_cast<Instruction>(Op0))
> - if (LHS->getOpcode() == Instruction::Div)
> + if (LHS->getOpcode() == Instruction::SDiv ||
> + LHS->getOpcode()==Instruction::UDiv ||
> + LHS->getOpcode()==Instruction::FDiv)
>
>
> *** This isn't quite right. This will miscompile ((X sdiv C1) udiv
> C2). You want something like:
>
>
> // (X / C1) / C2 -> X / (C1*C2)
> if (Instruction *LHS = dyn_cast<Instruction>(Op0))
> if (LHS->getOpcode() == I.getOpcode())
>
>
> which is also simpler.
Right. Done.
>
>
> if (ConstantInt *LHSRHS =
> dyn_cast<ConstantInt>(LHS->getOperand(1))) {
> - // (X / C1) / C2 -> X / (C1*C2)
> - return BinaryOperator::createDiv(LHS->getOperand(0),
> - ConstantExpr::getMul(RHS,
> LHSRHS));
> + return BinaryOperator::create(
> + Instruction::BinaryOps(LHS->getOpcode()),
> LHS->getOperand(0),
> + ConstantExpr::getMul(RHS,
> LHSRHS));
> }
>
>
> *** If you use I.getOpcode(), you can drop the BinaryOps cast.
>
>
Done.
>
>
> *** Why not have commonIDivTransforms call commonDivTransforms? It
> would be nicer to just have:
>
>
> + if (Instruction *Common = commonIDivTransforms(I))
> + return Common;
>
Done.
>
> Also, please try to stick with the established style when modifying
> existing code. In this case, putting the '*' in the right place,
> capitalizing variables, etc.
>
Old habits die hard. I've had it drilled into my head for decades that
putting the * next to the var name is the *wrong* place to put it as the
* modifies the type not the var. I'll try to retain style consistency,
however.
>
> + // Check to see if this is an unsigned division with an exact power
> of 2,
> + // if so, convert to a right shift.
> + // X udiv C^2 -> X >> C
> + if (ConstantInt *C = dyn_cast<ConstantInt>(Op1)) {
> + if (uint64_t Val = C->getZExtValue()) // Don't break X / 0
> + if (isPowerOf2_64(Val)) {
> + uint64_t C = Log2_64(Val);
> + return new ShiftInst(Instruction::Shr, Op0,
> + ConstantInt::get(Type::UByteTy, C));
> + }
> + }
>
>
> *** This will assert and die on something like 'udiv int %C, 64'. You
> need to insert casts of the input value and of the output result if
> the operands/result is signed. This specific issue goes away when
> shifts are split up.
Okay.
>
>
> + if (Instruction *RHSI = dyn_cast<Instruction>(I.getOperand(1))) {
> + // Turn A / (C1 << N), where C1 is "1<<C2" into A >> (N+C2) [udiv
> only].
> + if (RHSI->getOpcode() == Instruction::Shl &&
> + isa<ConstantInt>(RHSI->getOperand(0)) &&
> + RHSI->getOperand(0)->getType()->isUnsigned()) {
> + uint64_t C1 =
> cast<ConstantInt>(RHSI->getOperand(0))->getZExtValue();
> + if (isPowerOf2_64(C1)) {
> + uint64_t C2 = Log2_64(C1);
> + Value *Add = RHSI->getOperand(1);
> + if (C2) {
> + Constant *C2V = ConstantInt::get(Add->getType(), C2);
> + Add = InsertNewInstBefore(BinaryOperator::createAdd(Add,
> C2V,
> + "tmp"),
> I);
> }
> + return new ShiftInst(Instruction::Shr, Op0, Add);
> }
> }
> }
>
>
> *** This code doesn't need to check
> 'RHSI->getOperand(0)->getType()->isUnsigned()', but that will make you
> have to handle the signed case right (inserting casts).
Okay.
>
>
> + // If the sign bits of both operands are zero (i.e. we can prove
> they are
> + // unsigned inputs), turn this into a udiv.
> + uint64_t Mask = 1ULL << (I.getType()->getPrimitiveSizeInBits()-1);
> + if (MaskedValueIsZero(Op1, Mask) && MaskedValueIsZero(Op0, Mask)) {
> + const Type *NTy = Op0->getType()->getUnsignedVersion();
> + Instruction *LHS = new CastInst(Op0, NTy, Op0->getName());
> + InsertNewInstBefore(LHS, I);
> + Value *RHS;
> + if (Constant *R = dyn_cast<Constant>(Op1))
> + RHS = ConstantExpr::getCast(R, NTy);
> + else
> + RHS = InsertNewInstBefore(new CastInst(Op1, NTy,
> Op1->getName()), I);
> + Instruction *Div = BinaryOperator::createUDiv(LHS, RHS,
> I.getName());
> + InsertNewInstBefore(Div, I);
> + return new CastInst(Div, I.getType());
> + }
>
>
> *** This code gets much simpler now. Try:
>
>
> uint64_t Mask = 1ULL << (I.getType()->getPrimitiveSizeInBits()-1);
> if (MaskedValueIsZero(Op1, Mask) && MaskedValueIsZero(Op0, Mask))
> return BinaryOperator::createUDiv(LHS, RHS, I.getName());
actually ..
return BinaryOperator::createUDiv(Op0, Op1, I.getName());
but that's just being picky.
> The cast sequence *is* needed for converting stuff to shifts etc,
> above.
I'm not following this comment. What cast sequence? The one where we're
trying to turn it into a udiv if the sign bits are zero? If so, that
contradicts your simplification for this transform.
>
> + // Handle div X, Cond?Y:Z
> + if (SelectInst *SI = dyn_cast<SelectInst>(Op1)) {
> + // div X, (Cond ? 0 : Y) -> div X, Y. If the div and the select
> are in the
> + // same basic block, then we replace the select with Y, and the
> condition of
> + // the select with false (if the cond value is in the same BB).
> If the
> + // select has uses other than the div, this allows them to be
> simplified
> + // also.
> + if (Constant *ST = dyn_cast<Constant>(SI->getOperand(1)))
> + if (ST->isNullValue()) {
> + Instruction *CondI =
> dyn_cast<Instruction>(SI->getOperand(0));
> + if (CondI && CondI->getParent() == I.getParent())
> + UpdateValueUsesWith(CondI, ConstantBool::getFalse());
> + else if (I.getParent() != SI->getParent() || SI->hasOneUse())
> + I.setOperand(1, SI->getOperand(2));
> + else
> + UpdateValueUsesWith(SI, SI->getOperand(2));
> + return &I;
> + }
> +
> + // Likewise for: div X, (Cond ? Y : 0) -> div X, Y
> + if (Constant *ST = dyn_cast<Constant>(SI->getOperand(2)))
> + if (ST->isNullValue()) {
> + Instruction *CondI =
> dyn_cast<Instruction>(SI->getOperand(0));
> + if (CondI && CondI->getParent() == I.getParent())
> + UpdateValueUsesWith(CondI, ConstantBool::getTrue());
> + else if (I.getParent() != SI->getParent() || SI->hasOneUse())
> + I.setOperand(1, SI->getOperand(1));
> + else
> + UpdateValueUsesWith(SI, SI->getOperand(1));
> + return &I;
> + }
> + }
>
>
> *** This sequence should be in commonDivTransforms, no? You have
> cloned this code in commonIDivTransforms, please merge the two copies.
I was trying to preserve order of execution of the tests. I looked into
it and it doesn't matter so I merged it.
>
>
> Another significant issue not obvious from your diff is that you left
> this hunk of code:
>
>
> // If this is 'udiv X, (Cond ? C1, C2)' where C1&C2 are powers of
> two,
> // transform this into: '(Cond ? (udiv X, C1) : (udiv X, C2))'.
> if (ConstantInt *STO = dyn_cast<ConstantInt>(SI->getOperand(1)))
> if (ConstantInt *SFO =
> dyn_cast<ConstantInt>(SI->getOperand(2)))
> if (STO->getType()->isUnsigned() &&
> SFO->getType()->isUnsigned()) {
> // STO == 0 and SFO == 0 handled above.
> uint64_t TVA = STO->getZExtValue(), FVA =
> SFO->getZExtValue();
> if (isPowerOf2_64(TVA) && isPowerOf2_64(FVA)) {
> unsigned TSA = Log2_64(TVA), FSA = Log2_64(FVA);
> Constant *TC = ConstantInt::get(Type::UByteTy, TSA);
> Instruction *TSI = new ShiftInst(Instruction::Shr, Op0,
> TC, SI->getName()+".t");
> TSI = InsertNewInstBefore(TSI, I);
>
>
> Constant *FC = ConstantInt::get(Type::UByteTy, FSA);
> Instruction *FSI = new ShiftInst(Instruction::Shr, Op0,
> FC, SI->getName()+".f");
> FSI = InsertNewInstBefore(FSI, I);
> return new SelectInst(SI->getOperand(0), TSI, FSI);
> }
> }
>
>
> in the commonIDivTransforms method, but it is specific to udiv.
> Please move it to the udiv case, and make it insert casts etc as
> needed.
Actually, I think I lost it completely. I don't see this in my file. I
added it back to the udiv case as a separate transform. It now checks
for the STO==0 and SFO==0 cases because they are no longer "handled
above" (the "above" code is in commonDivTransforms).
>
>
> @@ -3720,11 +3803,13 @@ Instruction *InstCombiner::visitXor(Bina
> /// MulWithOverflow - Compute Result = In1*In2, returning true if the
> result
> /// overflowed for this type.
> static bool MulWithOverflow(ConstantInt *&Result, ConstantInt *In1,
> ConstantInt *In2) {
> Result = cast<ConstantInt>(ConstantExpr::getMul(In1, In2));
> - return !In2->isNullValue() && ConstantExpr::getDiv(Result, In2) !=
> In1;
> + return !In2->isNullValue() && (In2->getType()->isSigned() ?
> + ConstantExpr::getSDiv(Result, In2) :
> + ConstantExpr::getUDiv(Result, In2)) != In1;
> }
>
>
> static bool isPositive(ConstantInt *C) {
> return C->getSExtValue() >= 0;
> }
>
>
> *** This is subtly buggy. If you look at how MulWithOverflow is used,
> it is called from the "Fold: (div X, C1) op C2 -> range check" case.
> You need to pass in whether or not the *operation* is signed or
> unsigned, ignoring the sign of the values.
MulWithOverflow isn't used elsewhere in the file so I merged it into
visitSelectCC so its a little more clear what's going on. No point
making a function with 4 arguments called from only one place.
>
>
> @@ -4377,11 +4462,12 @@ Instruction *InstCombiner::visitSetCondI
> }
> }
> }
> break;
>
>
> - case Instruction::Div:
> + case Instruction::SDiv:
> + case Instruction::UDiv:
> // Fold: (div X, C1) op C2 -> range check
> if (ConstantInt *DivRHS =
> dyn_cast<ConstantInt>(LHSI->getOperand(1))) {
> // Fold this div into the comparison, producing a range
> check.
> // Determine, based on the divide type, what the range is
> being
> // checked. If there is an overflow on the low or high
> side, remember
>
>
> *** Likewise, this is extremely buggy. The original code uses
> knowledge of the signedness of the operands to determine the
> signedness of the comparisons it makes. For example, the code (which
> your diff doesn't include) has stuff like:
>
>
> } else if (LHSI->getType()->isUnsigned()) { // udiv
> LoBound = Prod;
> LoOverflow = ProdOV;
> HiOverflow = ProdOV || AddWithOverflow(HiBound, LoBound,
> DivRHS);
Changed the else if condition to LHSI->getOpcode() == Instruction::UDiv
>
> ... this is looking at the type of the operands, not the operation
> code, this *must* be fixed. Later it has:
>
>
> else if (HiOverflow)
> return new SetCondInst(Instruction::SetGE, X,
> LoBound);
> else if (LoOverflow)
> return new SetCondInst(Instruction::SetLT, X,
> HiBound);
>
>
> These create signed/unsigned comparisons where the sign matched the
> sign of the operator, because they follow the sign of the operands.
> This needs to be fixed to cast the operands to be the same sign as the
> sign of the opcode.
Okay, I"m not completely following this but I'll look into it in more
detail.
Reid.
More information about the llvm-commits
mailing list