[PATCH] D38494: [SCEV] Handling for ICmp occuring in the evolution chain.

[Jatin Bhateja via Phabricator via llvm-commits]

jbhateja added a comment.

In https://reviews.llvm.org/D38494#911259, @junryoungju wrote:

> In https://reviews.llvm.org/D38494#910903, @sanjoy wrote:
>
> > In https://reviews.llvm.org/D38494#910421, @junryoungju wrote:
> >
> > > :( that I commented didn't reviewed. this is why I wanted to make other reversion.
> >
> >
> > Are you saying that you had made the same comments as I did above?  If so, that wasn't clear from what you wrote.  You can help authors by:
> >
> > - Adding inline comments instead of a top level comment, knowing which part of the patch you're referring to helps comprehension
> > - Being specific; instead of saying "this is very general" say "use `cast` instead of `switch`"
>
>
> No, I wanted to say all that we are missing a very important point.
>
> In https://reviews.llvm.org/D38494#908808, @junryoungju wrote:
>
> > Okay, just we need to make sure we are evoluting conditional instructions or just checking that instructions are true or false for now.
> >
> > first one.
> >  If we are "actaully" evoluting a conditional instructions. better creating SCEVConditonal, checking that is true or false when is computing backedge taken count on each AddRec step.
> >  I think this is a correct way for handling conditional instructions.
> >  but, this will cause SCEV jobs MUUUUCHHH SLOWER.
> >
> > second one.
> >  If we are just checking that instructions are true or false. we do not need to rewrite it, rewriting value means that we handle that value as that "actual" SCEV. (register SCEV on SCEVCallbackVH)
> >  this mean, this is same to write this chain on to createSCEV that we did at first.
> >  so we have to handle it as a "except" of SCEV chain. better inlining code into createAddRecFromPHI.
>
>
> This is the actual point.  not only fixing a ICmp, this will not fix a "point" that bug reported.
>  and this will cause same bug, but from littlely different code.
>  It just fixing only that "specific" IR assembles in a very narrow range.


@junryoungju, it will be good if you append to the bug report  with other Patterns, I had made changes in my last revision to cover other case[s]too (I guess you added one another pattern in the mirror revision which you created from this patch) , we can generalise this or incremental addition can also be made.


https://reviews.llvm.org/D38494