[llvm] r269728 - [InstCombine] check vector elements before trying to transform LE/GE vector icmp (PR27756)
Sanjay Patel via llvm-commits
llvm-commits at lists.llvm.org
Tue May 17 12:59:22 PDT 2016
Hi Mike -
It's actually not PPC-specific AFAIK. The change broke a bunch of sanitizer
bots and caused PR27792 ( https://llvm.org/bugs/show_bug.cgi?id=27792 ).
Working on the fix now. Sorry for the breakage.
On Tue, May 17, 2016 at 1:53 PM, Mike Aizatsky <aizatsky at google.com> wrote:
> Sanjay,
>
> I think this change is breaking both ppc64 sanitizer build bots.
>
> First breakage:
>
> http://lab.llvm.org:8011/builders/sanitizer-ppc64be-linux/builds/2016
>
> Not sure why is it PPC specific.
>
>
>
> On Mon, May 16, 2016 at 6:04 PM Sanjay Patel via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: spatel
>> Date: Mon May 16 19:57:57 2016
>> New Revision: 269728
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=269728&view=rev
>> Log:
>> [InstCombine] check vector elements before trying to transform LE/GE
>> vector icmp (PR27756)
>>
>> Fix a bug introduced with rL269426 :
>> [InstCombine] canonicalize* LE/GE vector integer comparisons to LT/GT
>> (PR26701, PR26819)
>>
>> We were assuming that a ConstantDataVector / ConstantVector /
>> ConstantAggregateZero operand of
>> an ICMP was composed of ConstantInt elements, but it might have
>> ConstantExpr or UndefValue
>> elements. Handle those appropriately.
>>
>> Also, refactor this function to join the scalar and vector paths and
>> eliminate the switches.
>>
>> Differential Revision: http://reviews.llvm.org/D20289
>>
>> Modified:
>> llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
>> llvm/trunk/test/Transforms/InstCombine/icmp-vec.ll
>>
>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp?rev=269728&r1=269727&r2=269728&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
>> (original)
>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp Mon May
>> 16 19:57:57 2016
>> @@ -3102,90 +3102,54 @@ bool InstCombiner::replacedSelectWithOpe
>> /// If we have an icmp le or icmp ge instruction with a constant
>> operand, turn
>> /// it into the appropriate icmp lt or icmp gt instruction. This
>> transform
>> /// allows them to be folded in visitICmpInst.
>> -static ICmpInst *canonicalizeCmpWithConstant(ICmpInst &I,
>> - InstCombiner::BuilderTy
>> &Builder) {
>> +static ICmpInst *canonicalizeCmpWithConstant(ICmpInst &I) {
>> + ICmpInst::Predicate Pred = I.getPredicate();
>> + if (Pred != ICmpInst::ICMP_SLE && Pred != ICmpInst::ICMP_SGE &&
>> + Pred != ICmpInst::ICMP_ULE && Pred != ICmpInst::ICMP_UGE)
>> + return nullptr;
>> +
>> Value *Op0 = I.getOperand(0);
>> Value *Op1 = I.getOperand(1);
>> -
>> - if (auto *Op1C = dyn_cast<ConstantInt>(Op1)) {
>> - // For scalars, SimplifyICmpInst has already handled the edge cases
>> for us,
>> - // so we just assert on them.
>> - APInt Op1Val = Op1C->getValue();
>> - switch (I.getPredicate()) {
>> - case ICmpInst::ICMP_ULE:
>> - assert(!Op1C->isMaxValue(false)); // A <=u MAX -> TRUE
>> - return new ICmpInst(ICmpInst::ICMP_ULT, Op0, Builder.getInt(Op1Val
>> + 1));
>> - case ICmpInst::ICMP_SLE:
>> - assert(!Op1C->isMaxValue(true)); // A <=s MAX -> TRUE
>> - return new ICmpInst(ICmpInst::ICMP_SLT, Op0, Builder.getInt(Op1Val
>> + 1));
>> - case ICmpInst::ICMP_UGE:
>> - assert(!Op1C->isMinValue(false)); // A >=u MIN -> TRUE
>> - return new ICmpInst(ICmpInst::ICMP_UGT, Op0, Builder.getInt(Op1Val
>> - 1));
>> - case ICmpInst::ICMP_SGE:
>> - assert(!Op1C->isMinValue(true)); // A >=s MIN -> TRUE
>> - return new ICmpInst(ICmpInst::ICMP_SGT, Op0, Builder.getInt(Op1Val
>> - 1));
>> - default:
>> - return nullptr;
>> - }
>> - }
>> -
>> - // The usual vector types are ConstantDataVector. Exotic vector types
>> are
>> - // ConstantVector. Zeros are special. They all derive from Constant.
>> - if (isa<ConstantDataVector>(Op1) || isa<ConstantVector>(Op1) ||
>> - isa<ConstantAggregateZero>(Op1)) {
>> - Constant *Op1C = cast<Constant>(Op1);
>> - Type *Op1Type = Op1->getType();
>> - unsigned NumElts = Op1Type->getVectorNumElements();
>> -
>> - // Set the new comparison predicate and splat a vector of 1 or -1 to
>> - // increment or decrement the vector constants. But first, check
>> that no
>> - // elements of the constant vector would overflow/underflow when we
>> - // increment/decrement the constants.
>> - //
>> + auto *Op1C = dyn_cast<Constant>(Op1);
>> + if (!Op1C)
>> + return nullptr;
>> +
>> + // Check if the constant operand can be safely incremented/decremented
>> without
>> + // overflowing/underflowing. For scalars, SimplifyICmpInst has already
>> handled
>> + // the edge cases for us, so we just assert on them. For vectors, we
>> must
>> + // handle the edge cases.
>> + Type *Op1Type = Op1->getType();
>> + bool IsSigned = I.isSigned();
>> + bool IsLE = (Pred == ICmpInst::ICMP_SLE || Pred == ICmpInst::ICMP_ULE);
>> + if (auto *CI = dyn_cast<ConstantInt>(Op1C)) {
>> + // A <= MAX -> TRUE ; A >= MIN -> TRUE
>> + assert(IsLE ? !CI->isMaxValue(IsSigned) : !CI->isMinValue(IsSigned));
>> + } else if (Op1Type->isVectorTy()) {
>> // TODO? If the edge cases for vectors were guaranteed to be handled
>> as they
>> - // are for scalar, we could remove the min/max checks here. However,
>> to do
>> - // that, we would have to use insertelement/shufflevector to replace
>> edge
>> - // values.
>> -
>> - CmpInst::Predicate NewPred;
>> - Constant *OneOrNegOne = nullptr;
>> - switch (I.getPredicate()) {
>> - case ICmpInst::ICMP_ULE:
>> - for (unsigned i = 0; i != NumElts; ++i)
>> - if
>> (cast<ConstantInt>(Op1C->getAggregateElement(i))->isMaxValue(false))
>> - return nullptr;
>> - NewPred = ICmpInst::ICMP_ULT;
>> - OneOrNegOne = ConstantInt::get(Op1Type, 1);
>> - break;
>> - case ICmpInst::ICMP_SLE:
>> - for (unsigned i = 0; i != NumElts; ++i)
>> - if
>> (cast<ConstantInt>(Op1C->getAggregateElement(i))->isMaxValue(true))
>> - return nullptr;
>> - NewPred = ICmpInst::ICMP_SLT;
>> - OneOrNegOne = ConstantInt::get(Op1Type, 1);
>> - break;
>> - case ICmpInst::ICMP_UGE:
>> - for (unsigned i = 0; i != NumElts; ++i)
>> - if
>> (cast<ConstantInt>(Op1C->getAggregateElement(i))->isMinValue(false))
>> - return nullptr;
>> - NewPred = ICmpInst::ICMP_UGT;
>> - OneOrNegOne = ConstantInt::get(Op1Type, -1);
>> - break;
>> - case ICmpInst::ICMP_SGE:
>> - for (unsigned i = 0; i != NumElts; ++i)
>> - if
>> (cast<ConstantInt>(Op1C->getAggregateElement(i))->isMinValue(true))
>> - return nullptr;
>> - NewPred = ICmpInst::ICMP_SGT;
>> - OneOrNegOne = ConstantInt::get(Op1Type, -1);
>> - break;
>> - default:
>> - return nullptr;
>> - }
>> -
>> - return new ICmpInst(NewPred, Op0, ConstantExpr::getAdd(Op1C,
>> OneOrNegOne));
>> + // are for scalar, we could remove the min/max checks. However, to
>> do that,
>> + // we would have to use insertelement/shufflevector to replace edge
>> values.
>> + unsigned NumElts = Op1Type->getVectorNumElements();
>> + for (unsigned i = 0; i != NumElts; ++i) {
>> + Constant *Elt = Op1C->getAggregateElement(i);
>> + if (isa<UndefValue>(Elt))
>> + continue;
>> + // Bail out if we can't determine if this constant is min/max or
>> if we
>> + // know that this constant is min/max.
>> + auto *CI = dyn_cast<ConstantInt>(Elt);
>> + if (!CI || (IsLE ? CI->isMaxValue(IsSigned) :
>> CI->isMinValue(IsSigned)))
>> + return nullptr;
>> + }
>> + } else {
>> + // ConstantExpr?
>> + return nullptr;
>> }
>>
>> - return nullptr;
>> + // Increment or decrement the constant and set the new comparison
>> predicate:
>> + // ULE -> ULT ; UGE -> UGT ; SLE -> SLT ; SGE -> SGT
>> + Constant *OneOrNegOne = ConstantInt::get(Op1Type, IsLE ? 1 : -1);
>> + CmpInst::Predicate NewPred = IsLE ? ICmpInst::ICMP_ULT:
>> ICmpInst::ICMP_UGT;
>> + NewPred = IsSigned ? ICmpInst::getSignedPredicate(NewPred) : NewPred;
>> + return new ICmpInst(NewPred, Op0, ConstantExpr::getAdd(Op1C,
>> OneOrNegOne));
>> }
>>
>> Instruction *InstCombiner::visitICmpInst(ICmpInst &I) {
>> @@ -3271,7 +3235,7 @@ Instruction *InstCombiner::visitICmpInst
>> }
>> }
>>
>> - if (ICmpInst *NewICmp = canonicalizeCmpWithConstant(I, *Builder))
>> + if (ICmpInst *NewICmp = canonicalizeCmpWithConstant(I))
>> return NewICmp;
>>
>> unsigned BitWidth = 0;
>>
>> Modified: llvm/trunk/test/Transforms/InstCombine/icmp-vec.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/icmp-vec.ll?rev=269728&r1=269727&r2=269728&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/InstCombine/icmp-vec.ll (original)
>> +++ llvm/trunk/test/Transforms/InstCombine/icmp-vec.ll Mon May 16
>> 19:57:57 2016
>> @@ -159,3 +159,25 @@ define <2 x i1> @ule_max(<2 x i3> %x) {
>> ret <2 x i1> %cmp
>> }
>>
>> +; If we can't determine if a constant element is min/max (eg, it's a
>> ConstantExpr), do nothing.
>> +
>> +define <2 x i1> @PR27756_1(<2 x i8> %a) {
>> +; CHECK-LABEL: @PR27756_1(
>> +; CHECK-NEXT: [[CMP:%.*]] = icmp sle <2 x i8> %a, <i8 bitcast (<2 x
>> i4> <i4 1, i4 2> to i8), i8 0>
>> +; CHECK-NEXT: ret <2 x i1> [[CMP]]
>> +;
>> + %cmp = icmp sle <2 x i8> %a, <i8 bitcast (<2 x i4> <i4 1, i4 2> to
>> i8), i8 0>
>> + ret <2 x i1> %cmp
>> +}
>> +
>> +; Undef elements don't prevent the transform of the comparison.
>> +
>> +define <2 x i1> @PR27756_2(<2 x i8> %a) {
>> +; CHECK-LABEL: @PR27756_2(
>> +; CHECK-NEXT: [[CMP:%.*]] = icmp slt <2 x i8> %a, <i8 undef, i8 1>
>> +; CHECK-NEXT: ret <2 x i1> [[CMP]]
>> +;
>> + %cmp = icmp sle <2 x i8> %a, <i8 undef, i8 0>
>> + ret <2 x i1> %cmp
>> +}
>> +
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
> --
> Mike
> Sent from phone
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160517/1bc7e635/attachment.html>
More information about the llvm-commits
mailing list