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

Max Kazantsev via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 26 18:58:30 PDT 2018


mkazantsev added a comment.

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?




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?


I think that these changes are practically either unimportant or positive. SCEV has reasons to prefer `lt/gt` comparisons over `ne/eq` because in certain cases it can derive more facts from it, however I'm not sure whether it has real profit anywhere or not. I can test it on our Java benchmarks, but don't have environment to run C++ clang tests properly, so I'd appreciate if you could help with it.

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

> 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.


Interesting idea, I need to check it.


https://reviews.llvm.org/D48584





More information about the llvm-commits mailing list