[llvm] r314698 - [InstCombine] remove one-use restriction for icmp (shr exact X, C1), C2 --> icmp X, (C2<<C1)
Sanjay Patel via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 5 12:06:51 PDT 2017
The bot did go green after the revert, but as we've discussed, this is
almost certainly a latent bug somewhere else. If we can track that down, we
should be able to recommit this patch.
On Thu, Oct 5, 2017 at 8:29 AM, Sanjay Patel <spatel at rotateright.com> wrote:
> Reverted with:
>
> https://reviews.llvm.org/rL314984
>
> On Thu, Oct 5, 2017 at 7:28 AM, Diana Picus <diana.picus at linaro.org>
> wrote:
>
>> On 5 October 2017 at 15:22, Sanjay Patel <spatel at rotateright.com> wrote:
>> > No problem. I'm not sure if there's anything more I can do to help if I
>> > can't repro locally?
>>
>> No, I'll just have to figure it out.
>>
>> > If you'd like me to revert while we investigate, that's ok with me too.
>>
>> That would be nice, so we can get the bot back to green.
>>
>> > On Thu, Oct 5, 2017 at 6:33 AM, Diana Picus <diana.picus at linaro.org>
>> wrote:
>> >>
>> >> Sorry, I had to run off for dinner last night.
>> >>
>> >> I'm having trouble getting this to crash with llc :( I can't piece
>> >> together what combination of flags gets it to crash.
>> >> So it's a bit difficult to reduce. I'm currently just inspecting the
>> >> diffs.
>> >>
>> >>
>> >> On 4 October 2017 at 20:16, Sanjay Patel <spatel at rotateright.com>
>> wrote:
>> >> > Thinking out loud...we have this assertion while compiling this file,
>> >> > right?
>> >> >
>> >> >
>> >> >
>> >> > /home/buildslave/buildslave/clang-cmake-armv7-a15-selfhost-
>> neon/llvm/include/llvm/ADT/DenseMap.h:374:
>> >> > Assertion `!FoundVal && "Key already in new map?"' failed
>> >> >
>> >> >
>> >> > Can you use bugpoint to reduce the IR that causes the crash?
>> >> >
>> >> >
>> >> > On Wed, Oct 4, 2017 at 12:04 PM, Sanjay Patel <
>> spatel at rotateright.com>
>> >> > wrote:
>> >> >>
>> >> >> Another way to reduce this - if you manually edit the IR so that we
>> >> >> have
>> >> >> only one of those IR diffs and then create the binary from that IR,
>> you
>> >> >> could figure out exactly which machine diff is going wrong (assuming
>> >> >> the
>> >> >> crash goes away in all but one of these cases)?
>> >> >>
>> >> >> On Wed, Oct 4, 2017 at 11:58 AM, Sanjay Patel <
>> spatel at rotateright.com>
>> >> >> wrote:
>> >> >>>
>> >> >>> I think it must be going wrong later, but I'm not an ARM expert.
>> >> >>>
>> >> >>> This is a compare that includes a shift?
>> >> >>> CMPrsi killed %1191, %1190, 17, 14, _, implicit-def %cpsr
>> >> >>>
>> >> >>> and this is just a straight compare?
>> >> >>> CMPri %1190, 0, 14, _, implicit-def %cpsr
>> >> >>>
>> >> >>> I think we need to look at all of those diffs and see if we can
>> spot
>> >> >>> the
>> >> >>> logic bug down there.
>> >> >>>
>> >> >>>
>> >> >>> On Wed, Oct 4, 2017 at 11:42 AM, Diana Picus <
>> diana.picus at linaro.org>
>> >> >>> wrote:
>> >> >>>>
>> >> >>>> Ok then, any other ideas?
>> >> >>>>
>> >> >>>> On 4 October 2017 at 19:40, Sanjay Patel <spatel at rotateright.com>
>> >> >>>> wrote:
>> >> >>>> > The ashr + icmp transform is exactly what should have changed
>> with
>> >> >>>> > my
>> >> >>>> > patch,
>> >> >>>> > and AFAICT, it's logically correct:
>> >> >>>> > https://rise4fun.com/Alive/QmfT
>> >> >>>> >
>> >> >>>> > If 'nuw' disappears, that should be safe too.
>> >> >>>> >
>> >> >>>> > On Wed, Oct 4, 2017 at 11:29 AM, Diana Picus
>> >> >>>> > <diana.picus at linaro.org>
>> >> >>>> > wrote:
>> >> >>>> >>
>> >> >>>> >> (Also some nuw flags disappearing from adds)
>> >> >>>> >>
>> >> >>>> >> On 4 October 2017 at 19:28, Diana Picus <
>> diana.picus at linaro.org>
>> >> >>>> >> wrote:
>> >> >>>> >> > Hi Sanjay,
>> >> >>>> >> >
>> >> >>>> >> > I got the MIR before machine-cse at 314697 and 314698. If you
>> >> >>>> >> > vimdiff
>> >> >>>> >> > them you can see that pretty much the only difference at the
>> IR
>> >> >>>> >> > level
>> >> >>>> >> > is a lot of
>> >> >>>> >> > %sub.ptr.sub.i.i = sub i32
>> >> >>>> >> > %FixedEncodings.sroa.13.0.lcssa294.i271,
>> >> >>>> >> > %FixedEncodings.sroa.0.0.lcssa295.i270
>> >> >>>> >> > %sub.ptr.div.i.i = ashr exact i32 %sub.ptr.sub.i.i, 2
>> >> >>>> >> > %cmp40274.i = icmp eq i32 %sub.ptr.sub.i.i, 0
>> >> >>>> >> > instead of
>> >> >>>> >> > %sub.ptr.sub.i.i = sub i32
>> >> >>>> >> > %FixedEncodings.sroa.13.0.lcssa294.i271,
>> >> >>>> >> > %FixedEncodings.sroa.0.0.lcssa295.i270
>> >> >>>> >> > %sub.ptr.div.i.i = ashr exact i32 %sub.ptr.sub.i.i, 2
>> >> >>>> >> > %cmp40274.i = icmp eq i32 %sub.ptr.div.i.i, 0
>> >> >>>> >> >
>> >> >>>> >> > So it's using the result of the sub instead of the one of the
>> >> >>>> >> > ashr.
>> >> >>>> >> > Does that look like it could be caused by your patch?
>> >> >>>> >> >
>> >> >>>> >> > Cheers,
>> >> >>>> >> > Diana
>> >> >>>> >> >
>> >> >>>> >> > On 4 October 2017 at 17:08, Diana Picus <
>> diana.picus at linaro.org>
>> >> >>>> >> > wrote:
>> >> >>>> >> >> On 4 October 2017 at 16:30, Sanjay Patel
>> >> >>>> >> >> <spatel at rotateright.com>
>> >> >>>> >> >> wrote:
>> >> >>>> >> >>> Hi Diana -
>> >> >>>> >> >>>
>> >> >>>> >> >>> If I understand the config, it requires building on an ARM
>> >> >>>> >> >>> host
>> >> >>>> >> >>> to
>> >> >>>> >> >>> repro? I
>> >> >>>> >> >>> don't have that, so if you can create a reproducer more
>> >> >>>> >> >>> easily,
>> >> >>>> >> >>> that
>> >> >>>> >> >>> would
>> >> >>>> >> >>> be much appreciated!
>> >> >>>> >> >>
>> >> >>>> >> >> Ok, working on it.
>> >> >>>> >> >>
>> >> >>>> >> >>> I would have expected the whole world to die if the bug is
>> >> >>>> >> >>> directly in
>> >> >>>> >> >>> the
>> >> >>>> >> >>> patch I made, so it's more likely that an ARM-specific
>> backend
>> >> >>>> >> >>> bug has
>> >> >>>> >> >>> been
>> >> >>>> >> >>> exposed?
>> >> >>>> >> >>
>> >> >>>> >> >> That is very likely. I'll try to get some before / after
>> dumps
>> >> >>>> >> >> with
>> >> >>>> >> >> and without your patch, maybe we can narrow it down from
>> those.
>> >> >>>> >> >>
>> >> >>>> >> >>> On Wed, Oct 4, 2017 at 8:10 AM, Diana Picus
>> >> >>>> >> >>> <diana.picus at linaro.org>
>> >> >>>> >> >>> wrote:
>> >> >>>> >> >>>>
>> >> >>>> >> >>>> Hi Sanjay,
>> >> >>>> >> >>>>
>> >> >>>> >> >>>> I'm suspecting this is causing some trouble on one of our
>> >> >>>> >> >>>> selfhost
>> >> >>>> >> >>>> bots:
>> >> >>>> >> >>>>
>> >> >>>> >> >>>>
>> >> >>>> >> >>>>
>> >> >>>> >> >>>>
>> >> >>>> >> >>>> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-
>> selfhost-neon/builds/2117
>> >> >>>> >> >>>>
>> >> >>>> >> >>>> Could you please have a look? Let me know if you need me
>> to
>> >> >>>> >> >>>> send
>> >> >>>> >> >>>> a
>> >> >>>> >> >>>> reproducer.
>> >> >>>> >> >>>>
>> >> >>>> >> >>>> Thanks,
>> >> >>>> >> >>>> Diana
>> >> >>>> >> >>>>
>> >> >>>> >> >>>> On 2 October 2017 at 20:26, Sanjay Patel via llvm-commits
>> >> >>>> >> >>>> <llvm-commits at lists.llvm.org> wrote:
>> >> >>>> >> >>>> > Author: spatel
>> >> >>>> >> >>>> > Date: Mon Oct 2 11:26:44 2017
>> >> >>>> >> >>>> > New Revision: 314698
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > URL:
>> >> >>>> >> >>>> > http://llvm.org/viewvc/llvm-project?rev=314698&view=rev
>> >> >>>> >> >>>> > Log:
>> >> >>>> >> >>>> > [InstCombine] remove one-use restriction for icmp (shr
>> >> >>>> >> >>>> > exact
>> >> >>>> >> >>>> > X,
>> >> >>>> >> >>>> > C1), C2
>> >> >>>> >> >>>> > --> icmp X, (C2<<C1)
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > Modified:
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > llvm/trunk/lib/Transforms/Inst
>> Combine/InstCombineCompares.cpp
>> >> >>>> >> >>>> > llvm/trunk/test/Transforms/InstCombine/icmp-shr.ll
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > Modified:
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > llvm/trunk/lib/Transforms/Inst
>> Combine/InstCombineCompares.cpp
>> >> >>>> >> >>>> > URL:
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > http://llvm.org/viewvc/llvm-pr
>> oject/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompa
>> res.cpp?rev=314698&r1=314697&r2=314698&view=diff
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > ==============================
>> ================================================
>> >> >>>> >> >>>> > ---
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > llvm/trunk/lib/Transforms/Inst
>> Combine/InstCombineCompares.cpp
>> >> >>>> >> >>>> > (original)
>> >> >>>> >> >>>> > +++
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > llvm/trunk/lib/Transforms/Inst
>> Combine/InstCombineCompares.cpp
>> >> >>>> >> >>>> > Mon
>> >> >>>> >> >>>> > Oct 2 11:26:44 2017
>> >> >>>> >> >>>> > @@ -2058,15 +2058,15 @@ Instruction
>> >> >>>> >> >>>> > *InstCombiner::foldICmpShrCo
>> >> >>>> >> >>>> > (!IsAShr && C->shl(ShAmtVal).lshr(ShAmtVal)
>> ==
>> >> >>>> >> >>>> > *C))
>> >> >>>> >> >>>> > &&
>> >> >>>> >> >>>> > "Expected icmp+shr simplify did not occur.");
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > - // Check if the bits shifted out are known to be
>> zero.
>> >> >>>> >> >>>> > If
>> >> >>>> >> >>>> > so, we
>> >> >>>> >> >>>> > can
>> >> >>>> >> >>>> > compare
>> >> >>>> >> >>>> > - // against the unshifted value:
>> >> >>>> >> >>>> > + // If the bits shifted out are known zero, compare
>> the
>> >> >>>> >> >>>> > unshifted
>> >> >>>> >> >>>> > value:
>> >> >>>> >> >>>> > // (X & 4) >> 1 == 2 --> (X & 4) == 4.
>> >> >>>> >> >>>> > Constant *ShiftedCmpRHS =
>> >> >>>> >> >>>> > ConstantInt::get(Shr->getType(),
>> >> >>>> >> >>>> > *C <<
>> >> >>>> >> >>>> > ShAmtVal);
>> >> >>>> >> >>>> > - if (Shr->hasOneUse()) {
>> >> >>>> >> >>>> > - if (Shr->isExact())
>> >> >>>> >> >>>> > - return new ICmpInst(Pred, X, ShiftedCmpRHS);
>> >> >>>> >> >>>> > + if (Shr->isExact())
>> >> >>>> >> >>>> > + return new ICmpInst(Pred, X, ShiftedCmpRHS);
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > - // Otherwise strength reduce the shift into an
>> 'and'.
>> >> >>>> >> >>>> > + if (Shr->hasOneUse()) {
>> >> >>>> >> >>>> > + // Canonicalize the shift into an 'and':
>> >> >>>> >> >>>> > + // icmp eq/ne (shr X, ShAmt), C --> icmp eq/ne
>> (and X,
>> >> >>>> >> >>>> > HiMask), (C
>> >> >>>> >> >>>> > << ShAmt)
>> >> >>>> >> >>>> > APInt Val(APInt::getHighBitsSet(TypeBits,
>> TypeBits -
>> >> >>>> >> >>>> > ShAmtVal));
>> >> >>>> >> >>>> > Constant *Mask = ConstantInt::get(Shr->getType(),
>> >> >>>> >> >>>> > Val);
>> >> >>>> >> >>>> > Value *And = Builder.CreateAnd(X, Mask,
>> Shr->getName()
>> >> >>>> >> >>>> > +
>> >> >>>> >> >>>> > ".mask");
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > Modified:
>> >> >>>> >> >>>> > llvm/trunk/test/Transforms/InstCombine/icmp-shr.ll
>> >> >>>> >> >>>> > URL:
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > http://llvm.org/viewvc/llvm-pr
>> oject/llvm/trunk/test/Transforms/InstCombine/icmp-shr.ll?
>> rev=314698&r1=314697&r2=314698&view=diff
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > ==============================
>> ================================================
>> >> >>>> >> >>>> > --- llvm/trunk/test/Transforms/InstCombine/icmp-shr.ll
>> >> >>>> >> >>>> > (original)
>> >> >>>> >> >>>> > +++ llvm/trunk/test/Transforms/InstCombine/icmp-shr.ll
>> Mon
>> >> >>>> >> >>>> > Oct
>> >> >>>> >> >>>> > 2
>> >> >>>> >> >>>> > 11:26:44 2017
>> >> >>>> >> >>>> > @@ -483,7 +483,7 @@ declare void @foo(i32)
>> >> >>>> >> >>>> > define i1 @exact_multiuse(i32 %x) {
>> >> >>>> >> >>>> > ; CHECK-LABEL: @exact_multiuse(
>> >> >>>> >> >>>> > ; CHECK-NEXT: [[SH:%.*]] = lshr exact i32 %x, 7
>> >> >>>> >> >>>> > -; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 [[SH]], 1024
>> >> >>>> >> >>>> > +; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 %x, 131072
>> >> >>>> >> >>>> > ; CHECK-NEXT: call void @foo(i32 [[SH]])
>> >> >>>> >> >>>> > ; CHECK-NEXT: ret i1 [[CMP]]
>> >> >>>> >> >>>> > ;
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> >
>> >> >>>> >> >>>> > _______________________________________________
>> >> >>>> >> >>>> > 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/20171005/92a70f04/attachment.html>
More information about the llvm-commits
mailing list