[llvm] r324014 - [InstCombine] allow multi-use values in canEvaluate* if all uses are in 1 inst
Sanjay Patel via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 5 09:31:36 PST 2018
As Amjad noted in D42739 <https://reviews.llvm.org/D42739>, I failed to
account for select / PHI ops with multi-uses in this patch. I've attempted
to close that gap with:
https://reviews.llvm.org/rL324252
Please let me know if the inf-loop is gone with that change. I'm still
trying to create a test that would find that gap, but assuming that was the
bug, if we can get a reduction from whatever real program hit it, that
would be great. Thanks!
On Mon, Feb 5, 2018 at 7:28 AM, Sanjay Patel <spatel at rotateright.com> wrote:
> Thanks for letting me know. Sounds like an inf-loop.
>
> Since this patch is duplicating a transform that occurs in
> -aggressive-instcombine, it's possible that the bug already exists apart
> from this change, but might take the benign form of a transform reversal
> across passes, or it might be masked in that case.
>
> On Sat, Feb 3, 2018 at 8:02 PM, Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
>> On Thu, Feb 1, 2018 at 1:57 PM Sanjay Patel via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: spatel
>>> Date: Thu Feb 1 13:55:53 2018
>>> New Revision: 324014
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=324014&view=rev
>>> Log:
>>> [InstCombine] allow multi-use values in canEvaluate* if all uses are in
>>> 1 inst
>>>
>>> This is the enhancement suggested in D42536 to fix a shortcoming in
>>> regular InstCombine's canEvaluate* functionality.
>>> When we have multiple uses of a value, but they're all in one
>>> instruction, we can
>>> allow that expression to be narrowed or widened for the same cost as a
>>> single-use
>>> value.
>>>
>>> AFAICT, this can only matter for multiply: sub/and/or/xor/select would
>>> be simplified
>>> away if the operands are the same value; add becomes shl; shifts with a
>>> variable shift
>>> amount aren't handled.
>>>
>>
>> FWIW, we have at least one case which is now either inf-looping or taking
>> dramatically longer to compile after this commit (way over 2x from what I
>> can tell).
>>
>> We're working on building a test case, but just wanted to send a heads-up
>> in case others are hitting slow compiles.
>>
>>
>>>
>>> Differential Revision: https://reviews.llvm.org/D42739
>>>
>>> Modified:
>>> llvm/trunk/lib/Transforms/InstCombine/InstCombineCasts.cpp
>>> llvm/trunk/test/Transforms/InstCombine/cast-mul-select.ll
>>> llvm/trunk/test/Transforms/InstCombine/icmp-mul-zext.ll
>>>
>>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCasts.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transform
>>> s/InstCombine/InstCombineCasts.cpp?rev=324014&r1=324013&r2=
>>> 324014&view=diff
>>> ============================================================
>>> ==================
>>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCasts.cpp
>>> (original)
>>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCasts.cpp Thu Feb
>>> 1 13:55:53 2018
>>> @@ -185,8 +185,14 @@ Value *InstCombiner::EvaluateInDifferent
>>> case Instruction::Shl:
>>> case Instruction::UDiv:
>>> case Instruction::URem: {
>>> - Value *LHS = EvaluateInDifferentType(I->getOperand(0), Ty,
>>> isSigned);
>>> - Value *RHS = EvaluateInDifferentType(I->getOperand(1), Ty,
>>> isSigned);
>>> + Value *LHS, *RHS;
>>> + if (I->getOperand(0) == I->getOperand(1)) {
>>> + // Don't create an unnecessary value if the operands are repeated.
>>> + LHS = RHS = EvaluateInDifferentType(I->getOperand(0), Ty,
>>> isSigned);
>>> + } else {
>>> + LHS = EvaluateInDifferentType(I->getOperand(0), Ty, isSigned);
>>> + RHS = EvaluateInDifferentType(I->getOperand(1), Ty, isSigned);
>>> + }
>>> Res = BinaryOperator::Create((Instruction::BinaryOps)Opc, LHS,
>>> RHS);
>>> break;
>>> }
>>> @@ -320,10 +326,12 @@ static bool canNotEvaluateInType(Value *
>>> assert(!isa<Constant>(V) && "Constant should already be handled.");
>>> if (!isa<Instruction>(V))
>>> return true;
>>> - // We can't extend or shrink something that has multiple uses: doing
>>> so would
>>> - // require duplicating the instruction in general, which isn't
>>> profitable.
>>> + // We can't extend or shrink something that has multiple uses --
>>> unless those
>>> + // multiple uses are all in the same instruction -- doing so would
>>> require
>>> + // duplicating the instruction which isn't profitable.
>>> if (!V->hasOneUse())
>>> - return true;
>>> + if (any_of(V->users(), [&](User *U) { return U != V->user_back();
>>> }))
>>> + return true;
>>>
>>> return false;
>>> }
>>>
>>> Modified: llvm/trunk/test/Transforms/InstCombine/cast-mul-select.ll
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transfor
>>> ms/InstCombine/cast-mul-select.ll?rev=324014&r1=324013
>>> &r2=324014&view=diff
>>> ============================================================
>>> ==================
>>> --- llvm/trunk/test/Transforms/InstCombine/cast-mul-select.ll (original)
>>> +++ llvm/trunk/test/Transforms/InstCombine/cast-mul-select.ll Thu Feb
>>> 1 13:55:53 2018
>>> @@ -49,11 +49,9 @@ define i8 @select2(i1 %cond, i8 %x, i8 %
>>>
>>> define i32 @eval_trunc_multi_use_in_one_inst(i32 %x) {
>>> ; CHECK-LABEL: @eval_trunc_multi_use_in_one_inst(
>>> -; CHECK-NEXT: [[Z:%.*]] = zext i32 [[X:%.*]] to i64
>>> -; CHECK-NEXT: [[A:%.*]] = add nuw nsw i64 [[Z]], 15
>>> -; CHECK-NEXT: [[M:%.*]] = mul i64 [[A]], [[A]]
>>> -; CHECK-NEXT: [[T:%.*]] = trunc i64 [[M]] to i32
>>> -; CHECK-NEXT: ret i32 [[T]]
>>> +; CHECK-NEXT: [[A:%.*]] = add i32 [[X:%.*]], 15
>>> +; CHECK-NEXT: [[M:%.*]] = mul i32 [[A]], [[A]]
>>> +; CHECK-NEXT: ret i32 [[M]]
>>> ;
>>> %z = zext i32 %x to i64
>>> %a = add nsw nuw i64 %z, 15
>>> @@ -64,11 +62,9 @@ define i32 @eval_trunc_multi_use_in_one_
>>>
>>> define i32 @eval_zext_multi_use_in_one_inst(i32 %x) {
>>> ; CHECK-LABEL: @eval_zext_multi_use_in_one_inst(
>>> -; CHECK-NEXT: [[T:%.*]] = trunc i32 [[X:%.*]] to i16
>>> -; CHECK-NEXT: [[A:%.*]] = and i16 [[T]], 5
>>> -; CHECK-NEXT: [[M:%.*]] = mul nuw nsw i16 [[A]], [[A]]
>>> -; CHECK-NEXT: [[R:%.*]] = zext i16 [[M]] to i32
>>> -; CHECK-NEXT: ret i32 [[R]]
>>> +; CHECK-NEXT: [[A:%.*]] = and i32 [[X:%.*]], 5
>>> +; CHECK-NEXT: [[M:%.*]] = mul nuw nsw i32 [[A]], [[A]]
>>> +; CHECK-NEXT: ret i32 [[M]]
>>> ;
>>> %t = trunc i32 %x to i16
>>> %a = and i16 %t, 5
>>> @@ -79,12 +75,10 @@ define i32 @eval_zext_multi_use_in_one_i
>>>
>>> define i32 @eval_sext_multi_use_in_one_inst(i32 %x) {
>>> ; CHECK-LABEL: @eval_sext_multi_use_in_one_inst(
>>> -; CHECK-NEXT: [[T:%.*]] = trunc i32 [[X:%.*]] to i16
>>> -; CHECK-NEXT: [[A:%.*]] = and i16 [[T]], 14
>>> -; CHECK-NEXT: [[M:%.*]] = mul nuw nsw i16 [[A]], [[A]]
>>> -; CHECK-NEXT: [[O:%.*]] = or i16 [[M]], -32768
>>> -; CHECK-NEXT: [[R:%.*]] = sext i16 [[O]] to i32
>>> -; CHECK-NEXT: ret i32 [[R]]
>>> +; CHECK-NEXT: [[A:%.*]] = and i32 [[X:%.*]], 14
>>> +; CHECK-NEXT: [[M:%.*]] = mul nuw nsw i32 [[A]], [[A]]
>>> +; CHECK-NEXT: [[O:%.*]] = or i32 [[M]], -32768
>>> +; CHECK-NEXT: ret i32 [[O]]
>>> ;
>>> %t = trunc i32 %x to i16
>>> %a = and i16 %t, 14
>>>
>>> Modified: llvm/trunk/test/Transforms/InstCombine/icmp-mul-zext.ll
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transfor
>>> ms/InstCombine/icmp-mul-zext.ll?rev=324014&r1=324013&r2=324014&view=diff
>>> ============================================================
>>> ==================
>>> --- llvm/trunk/test/Transforms/InstCombine/icmp-mul-zext.ll (original)
>>> +++ llvm/trunk/test/Transforms/InstCombine/icmp-mul-zext.ll Thu Feb 1
>>> 13:55:53 2018
>>> @@ -55,13 +55,12 @@ lor.end:
>>>
>>> define void @PR33765(i8 %beth) {
>>> ; CHECK-LABEL: @PR33765(
>>> -; CHECK-NEXT: [[CONV:%.*]] = zext i8 [[BETH:%.*]] to i32
>>> +; CHECK-NEXT: [[CONV:%.*]] = zext i8 [[BETH:%.*]] to i16
>>> ; CHECK-NEXT: br i1 false, label [[IF_THEN9:%.*]], label [[IF_THEN9]]
>>> ; CHECK: if.then9:
>>> -; CHECK-NEXT: [[MUL:%.*]] = mul nuw nsw i32 [[CONV]], [[CONV]]
>>> +; CHECK-NEXT: [[MUL:%.*]] = mul nuw i16 [[CONV]], [[CONV]]
>>> ; CHECK-NEXT: [[TINKY:%.*]] = load i16, i16* @glob, align 2
>>> -; CHECK-NEXT: [[TMP1:%.*]] = trunc i32 [[MUL]] to i16
>>> -; CHECK-NEXT: [[CONV14:%.*]] = and i16 [[TINKY]], [[TMP1]]
>>> +; CHECK-NEXT: [[CONV14:%.*]] = and i16 [[TINKY]], [[MUL]]
>>> ; CHECK-NEXT: store i16 [[CONV14]], i16* @glob, align 2
>>> ; CHECK-NEXT: ret void
>>> ;
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180205/fefb8243/attachment.html>
More information about the llvm-commits
mailing list