[PATCH] D33172: [InstCombine] Simpify inverted predicates in 'or'

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D33172#760113, @modocache wrote:

> Rebased onto https://reviews.llvm.org/rL303133. Sorry, I'm a new contributor, so I'm having trouble understanding how to proceed here. Is there anything specifically I should change in this diff? Should I be using the isCanonicalPredicate helper added in https://reviews.llvm.org/rL303261? Or should I be using a different approach altogether?


It turns out this simple patch raises deep philosophical questions for LLVM. :)
http://lists.llvm.org/pipermail/llvm-dev/2017-May/113184.html

I agreed with Davide that the patch as written is too specific. We should handle this problem more generally, and likely that's a shortcoming of some other pass. IMO and regardless of that, InstCombine is still missing a canonicalization of the cmp predicate that is feeding the select. I did not see any other comments to know if anyone else agrees or disagrees with that position. If you disagree with that, then I assume you would also want to see the related cmp+branch canonicalization removed? I suspect that would require some thorough perf analysis to justify.

But since we have that canonicalization today, I think the immediate problem will be solved either here in InstCombine or somewhere else using existing logic.

There's also a question about whether the existing fold that I pointed out:
// (op (select (a, c, b)), (select (a, d, b))) -> (select (a, (op c, d), 0))
should exist in InstCombine. Again, whether you think that belongs in InstCombine or not, IMO is irrelevant to the immediate goal of solving PR32791 because the fold already exists, and there's no evidence that some other pass handles that case. If you advocate removing that, then you have to come up with a better solution...which could be a major undertaking at this point.

So to answer the question: yes, I think we should use the helper added in https://reviews.llvm.org/rL303261 to canonicalize cmps used by selects. However, doing that likely requires not introducing any known regressions to instcombine, and I already know there will be one. :)

Let me see if I can solve that one non-controversially and report back here on that.


https://reviews.llvm.org/D33172