[PATCH] D48584: [InstCombine] Delay foldICmpUsingKnownBits until simple transforms are done

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D48584#1143518, @spatel wrote:

> In https://reviews.llvm.org/D48584#1143257, @mkazantsev wrote:
>
> > In https://reviews.llvm.org/D48584#1143240, @lebedev.ri wrote:
> >
> > > This only moves the `foldICmpUsingKnownBits()` call later on,
> > >  but in PR37636 <https://bugs.llvm.org/show_bug.cgi?id=37636#c4> @spatel suggests it *might* expose more problems.
> > >  Does this expose any problems?
> >
> >
> > From my point of view, no. We use different predicates in some cases, I am not convinced that either of them is better (at least less/greater predicates are more useful for SCEV analysis than ne/eq).
> >  I'll fix the comments.
>
>
> The test diffs here show what I was concerned about. It's not clear to me if/when the predicate/constant changes in these compares are wins or not. It's probably in the noise, but we should check for regressions on test-suite or some other benchmark?


Also, are there more diffs if you sink foldICmpUsingKnownBits even further down? If there aren't, we should sink it all the way to the end because it's expensive in compile-time. Either way, please add a code comment, so we know why we positioned it wherever it is.


https://reviews.llvm.org/D48584