[llvm] r185257 - InstCombine: Be more agressive optimizing 'udiv' instrs with 'select' denoms

[Hal Finkel]

----- Original Message -----
> 
> 
> I can check all the operands twice or just not do the transform. I
> don't readily see a nicer alternative but I am receptive to
> suggestions!

In the mean time, I've reverted this in r185415. The current implementation is causing optimizer crashes. Given that the asserts seem related to use references, I imagine that this will go away once you've removed the create-and-delete aspect of the current implementation. I've committed a regression test.

Thanks again,
Hal

> 
> 
> I figured that divides are worth annihilating with vigor due to how
> expensive they are and hoped they would be sufficiently uncommon
> that any wasted effort would be in the noise.
> 
> 
> If its any consolation, this transform fires on LLVM itself.
> 
> Sent from my iPhone
> 
> On Jun 29, 2013, at 9:15 AM, Eli Friedman < eli.friedman at gmail.com >
> wrote:
> 
> 
> 
> 
> 
> 
> 
> On Sat, Jun 29, 2013 at 1:40 AM, David Majnemer <
> david.majnemer at gmail.com > wrote:!
> 
> 
> Author: majnemer
> Date: Sat Jun 29 03:40:07 2013
> New Revision: 185257
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=185257&view=rev
> Log:
> InstCombine: Be more agressive optimizing 'udiv' instrs with 'select'
> denoms
> 
> Real world code sometimes has the denominator of a 'udiv' be a
> 'select'. LLVM can handle such cases but only when the 'select'
> operands are symmetric in structure (both select operands are a
> constant
> power of two or a left shift, etc.). This falls apart if we are dealt
> a
> 'udiv' where the code is not symetric or if the select operands lead
> us
> to more select instructions.
> 
> Instead, we should treat the LHS and each select operand as a
> distinct
> divide operation and try to optimize them independently. If we can
> to simplify each operation, then we can replace the 'udiv' with, say,
> a
> 'lshr' that has a new select with a bunch of new operands for the
> select.
> 
> Modified:
> llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
> llvm/trunk/test/Transforms/InstCombine/div-shift.ll
> 
> Modified:
> llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp?rev=185257&r1=185256&r2=185257&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
> (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
> Sat Jun 29 03:40:07 2013
> @@ -705,26 +705,27 @@ static Value *dyn_castZExtVal(Value *V,
> return 0;
> }
> 
> -Instruction *InstCombiner::visitUDiv(BinaryOperator &I) {
> - Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
> -
> - if (Value *V = SimplifyUDivInst(Op0, Op1, TD))
> - return ReplaceInstUsesWith(I, V);
> -
> - // Handle the integer div common cases
> - if (Instruction *Common = commonIDivTransforms(I))
> - return Common;
> +const unsigned MaxDepth = 6;
> 
> +// \brief Recursively visits the possible right hand operands of a
> udiv
> +// instruction, seeing through select instructions, to determine if
> we can
> +// replace the udiv with something simpler. If we find that an
> operand is not
> +// able to simplify the udiv, we abort the entire transformation.
> +//
> +// Inserts any intermediate instructions used for the simplification
> into
> +// NewInstrs and returns a new instruction that depends upon them.
> +static Instruction *visitUDivOperand(Value *Op0, Value *Op1,
> + const BinaryOperator &I,
> + SmallVectorImpl<Instruction *> &NewInstrs,
> + unsigned Depth = 0) {
> {
> // X udiv 2^C -> X >> C
> // Check to see if this is an unsigned division with an exact power
> of 2,
> // if so, convert to a right shift.
> const APInt *C;
> if (match(Op1, m_Power2(C))) {
> - BinaryOperator *LShr =
> - BinaryOperator::CreateLShr(Op0,
> - ConstantInt::get(Op0->getType(),
> - C->logBase2()));
> + BinaryOperator *LShr = BinaryOperator::CreateLShr(
> + Op0, ConstantInt::get(Op0->getType(), C->logBase2()));
> if (I.isExact()) LShr->setIsExact();
> return LShr;
> }
> @@ -733,51 +734,68 @@ Instruction *InstCombiner::visitUDiv(Bin
> if (ConstantInt *C = dyn_cast<ConstantInt>(Op1)) {
> // X udiv C, where C >= signbit
> if (C->getValue().isNegative()) {
> - Value *IC = Builder->CreateICmpULT(Op0, C);
> + ICmpInst *IC = new ICmpInst(ICmpInst::ICMP_ULT, Op0, C);
> + NewInstrs.push_back(IC);
> +
> return SelectInst::Create(IC, Constant::getNullValue(I.getType()),
> ConstantInt::get(I.getType(), 1));
> }
> }
> 
> - // (x lshr C1) udiv C2 --> x udiv (C2 << C1)
> - if (ConstantInt *C2 = dyn_cast<ConstantInt>(Op1)) {
> - Value *X;
> - ConstantInt *C1;
> - if (match(Op0, m_LShr(m_Value(X), m_ConstantInt(C1)))) {
> - APInt NC =
> C2->getValue().shl(C1->getLimitedValue(C1->getBitWidth()-1));
> - return BinaryOperator::CreateUDiv(X, Builder->getInt(NC));
> - }
> - }
> -
> // X udiv (C1 << N), where C1 is "1<<C2" --> X >> (N+C2)
> { const APInt *CI; Value *N;
> if (match(Op1, m_Shl(m_Power2(CI), m_Value(N))) ||
> match(Op1, m_ZExt(m_Shl(m_Power2(CI), m_Value(N))))) {
> - if (*CI != 1)
> - N = Builder->CreateAdd(N,
> - ConstantInt::get(N->getType(), CI->logBase2()));
> - if (ZExtInst *Z = dyn_cast<ZExtInst>(Op1))
> - N = Builder->CreateZExt(N, Z->getDestTy());
> - if (I.isExact())
> - return BinaryOperator::CreateExactLShr(Op0, N);
> - return BinaryOperator::CreateLShr(Op0, N);
> + if (*CI != 1) {
> + N = BinaryOperator::CreateAdd(
> + N, ConstantInt::get(N->getType(), CI->logBase2()));
> + NewInstrs.push_back(cast<Instruction>(N));
> + }
> + if (ZExtInst *Z = dyn_cast<ZExtInst>(Op1)) {
> + N = new ZExtInst(N, Z->getDestTy());
> + NewInstrs.push_back(cast<Instruction>(N));
> + }
> + BinaryOperator *LShr = BinaryOperator::CreateLShr(Op0, N);
> + if (I.isExact()) LShr->setIsExact();
> + return LShr;
> }
> }
> 
> - // udiv X, (Select Cond, C1, C2) --> Select Cond, (shr X, C1), (shr
> X, C2)
> - // where C1&C2 are powers of two.
> - { Value *Cond; const APInt *C1, *C2;
> - if (match(Op1, m_Select(m_Value(Cond), m_Power2(C1),
> m_Power2(C2)))) {
> - // Construct the "on true" case of the select
> - Value *TSI = Builder->CreateLShr(Op0, C1->logBase2(),
> Op1->getName()+".t",
> - I.isExact());
> -
> - // Construct the "on false" case of the select
> - Value *FSI = Builder->CreateLShr(Op0, C2->logBase2(),
> Op1->getName()+".f",
> - I.isExact());
> + // The remaining tests are all recursive, so bail out if we hit the
> limit.
> + if (Depth++ == MaxDepth)
> + return 0;
> +
> + if (SelectInst *SI = dyn_cast<SelectInst>(Op1))
> + if (Instruction *LHS =
> + visitUDivOperand(Op0, SI->getOperand(1), I, NewInstrs)) {
> + NewInstrs.push_back(LHS);
> + if (Instruction *RHS =
> + visitUDivOperand(Op0, SI->getOperand(2), I, NewInstrs)) {
> + NewInstrs.push_back(RHS);
> + return SelectInst::Create(SI->getCondition(), LHS, RHS);
> + }
> + }
> +
> + return 0;
> +}
> +
> +Instruction *InstCombiner::visitUDiv(BinaryOperator &I) {
> + Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
> +
> + if (Value *V = SimplifyUDivInst(Op0, Op1, TD))
> + return ReplaceInstUsesWith(I, V);
> +
> + // Handle the integer div common cases
> + if (Instruction *Common = commonIDivTransforms(I))
> + return Common;
> 
> - // construct the select instruction and return it.
> - return SelectInst::Create(Cond, TSI, FSI);
> + // (x lshr C1) udiv C2 --> x udiv (C2 << C1)
> + if (ConstantInt *C2 = dyn_cast<ConstantInt>(Op1)) {
> + Value *X;
> + ConstantInt *C1;
> + if (match(Op0, m_LShr(m_Value(X), m_ConstantInt(C1)))) {
> + APInt NC =
> C2->getValue().shl(C1->getLimitedValue(C1->getBitWidth()-1));
> + return BinaryOperator::CreateUDiv(X, Builder->getInt(NC));
> }
> }
> 
> @@ -788,6 +806,21 @@ Instruction *InstCombiner::visitUDiv(Bin
> I.isExact()),
> I.getType());
> 
> + // (LHS udiv (select (select (...)))) -> (LHS >> (select (select
> (...))))
> + SmallVector<Instruction *, 4> NewInstrs;
> + Instruction *RetI = visitUDivOperand(Op0, Op1, I, NewInstrs);
> + for (unsigned i = 0, e = NewInstrs.size(); i != e; i++)
> + // If we managed to replace the UDiv completely, insert the new
> intermediate
> + // instructions before where the UDiv was.
> + // If we couldn't, we must clean up after ourselves by deleting the
> new
> + // instructions.
> + if (RetI)
> + NewInstrs[i]->insertBefore(&I);
> + else
> + delete NewInstrs[i];
> + if (RetI)
> + return RetI;
> 
> 
> 
> 
> "Create a bunch of instructions, then delete them" is a pattern we
> generally try to avoid in InstCombine. I can see why you chose it
> here, but please don't; it's a bad idea in terms of performance.
> 
> 
> -Eli
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory