[PATCH] D42417: [SCEV] Fix isLoopEntryGuardedByCond

[Sanjoy Das via llvm-commits]

On Thu, Jan 25, 2018 at 9:34 PM, Serguei Katkov <serguei.katkov at azul.com> wrote:
> Hmmm, why do look into howManyLessThans?
>
> I guess the (2) is about ScalarEvolution::getSignExtendExpr...
>
>
> do I miss anything?

No, I did. :)

But even in getSignExtendExpr a crash in isLoopEntryGuardedByCond(L,
Pred, Start, OverflowLimit) would be weird (unless I'm misreading
again) since the LHS is the start of an add recurrence on L.

-- Sanjoy

>
> Thank you,
> Serguei.
>
>> -----Original Message-----
>> From: Sanjoy Das [mailto:sanjoy at playingwithpointers.com]
>> Sent: Friday, January 26, 2018 12:15 PM
>> To: reviews+D42417+public+54a7b9b7b0be118c at reviews.llvm.org
>> Cc: Serguei Katkov <serguei.katkov at azul.com>; Maxim Kazantsev
>> <max.kazantsev at azul.com>; Anna Thomas <anna at azul.com>;
>> dorit.nuzman at intel.com; Philip Reames <listmail at philipreames.com>; llvm-
>> commits <llvm-commits at lists.llvm.org>; Junbum Lim
>> <junbuml at codeaurora.org>; Wei Mi <wmi at google.com>;
>> florian.hahn at arm.com
>> Subject: Re: [PATCH] D42417: [SCEV] Fix isLoopEntryGuardedByCond
>>
>> I clicked through (2) and that does look suspicious -- the only call to
>> isLoopEntryGuardedByCond in howManyLessThans is
>>
>>   // as if the backedge is taken at least once max(End,Start) is End and so the
>>   // result is as above, and if not max(End,Start) is Start so we get a backedge
>>   // count of zero.
>>   const SCEV *BECount;
>>   if (isLoopEntryGuardedByCond(L, Cond, getMinusSCEV(Start, Stride), RHS))
>>     BECount = BECountIfBackedgeTaken;
>>   else {
>>     End = IsSigned ? getSMaxExpr(RHS, Start) : getUMaxExpr(RHS, Start);
>>     BECount = computeBECount(getMinusSCEV(End, Start), Stride, false);
>>
>> but we have
>>
>>   // Avoid weird loops
>>   if (!IV || IV->getLoop() != L || !IV->isAffine())
>>     return getCouldNotCompute();
>>
>>   ...
>>   const SCEV *Start = IV->getStart();
>>
>>   ...
>>   const SCEV *Stride = IV->getStepRecurrence(*this);
>>
>>
>> I may be missing something, but altogether this should imply that
>> getMinusSCEV(Start, Stride) is, in fact, available at the loop entry.
>>
>> -- Sanjoy
>>
>> On Wed, Jan 24, 2018 at 9:19 PM, Serguei Katkov via Phabricator
>> <reviews at reviews.llvm.org> wrote:
>> > skatkov added a comment.
>> >
>> > Hi Sanjoy,
>> >
>> > The number of buildbot failures were so big but I remember the following
>> case (if I do not miss anything):
>> >
>> > 1. All Usages in IRCE caused an assert in unit testing 2. Crash was in
>> > getSignExtendExpr: the prblem was with LHS. Might be it is the same
>> > issue as 3rd one. Example:
>> > http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/14076/steps
>> > /ninja%20check%201/logs/stdio or
>> > http://lab.llvm.org:8011/builders/clang-s390x-linux-lnt/builds/4265/st
>> > eps/ninja%20check%201/logs/stdio 3. Most popular
>> > ScalarEvolution::isImpliedCondOperandsViaNoOverflow (LHS). Example :
>> > http://lab.llvm.org:8011/builders/clang-cmake-x86_64-avx2-linux-perf/b
>> > uilds/3854/steps/test-suite/logs/test.log
>> >
>> > I hope it helps.
>> >
>> >
>> > https://reviews.llvm.org/D42417
>> >
>> >
>> >