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