[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 07:29:41 PDT 2017
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/InstCombine/
> InstCombineCompares.cpp
> >> >>>> >> >>>> > llvm/trunk/test/Transforms/InstCombine/icmp-shr.ll
> >> >>>> >> >>>> >
> >> >>>> >> >>>> > Modified:
> >> >>>> >> >>>> >
> >> >>>> >> >>>> > llvm/trunk/lib/Transforms/InstCombine/
> InstCombineCompares.cpp
> >> >>>> >> >>>> > URL:
> >> >>>> >> >>>> >
> >> >>>> >> >>>> >
> >> >>>> >> >>>> >
> >> >>>> >> >>>> > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Transforms/InstCombine/InstCombineCompares.cpp?rev=
> 314698&r1=314697&r2=314698&view=diff
> >> >>>> >> >>>> >
> >> >>>> >> >>>> >
> >> >>>> >> >>>> >
> >> >>>> >> >>>> >
> >> >>>> >> >>>> > ==============================
> ================================================
> >> >>>> >> >>>> > ---
> >> >>>> >> >>>> >
> >> >>>> >> >>>> > llvm/trunk/lib/Transforms/InstCombine/
> InstCombineCompares.cpp
> >> >>>> >> >>>> > (original)
> >> >>>> >> >>>> > +++
> >> >>>> >> >>>> >
> >> >>>> >> >>>> > llvm/trunk/lib/Transforms/InstCombine/
> 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-project/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/6f7e714e/attachment.html>
More information about the llvm-commits
mailing list