[PATCH] D45862: [InstCombine] Relax restriction in foldSelectInstWithICmp for sake of smaller code size

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D45862#1074819, @mkazantsev wrote:

> For now, my point is the following: yes, we are *already* doing all this bit magic. Yes, it looks like miscanonicalization, and we can discuss getting rid of all such transforms at all. But since we are already doing it and haven't yet decided to clear it off, let's at least do it well. Once we decide that all this stuff is counter-canonical, we can happily remove all this. What do you think of that?


I understand this as a short-term goal and local improvement, but I think if we don't reverse the direction now, we'll never correct it. For example, it took over 5 years to correct:
https://reviews.llvm.org/rL159230
with
https://reviews.llvm.org/rL319964

See also the mentioned commits where we may extend the DAG transforms and possibly solve the patterns that you are looking at:
https://reviews.llvm.org/rL296977
https://reviews.llvm.org/rL311731

You mention the vectorizers as a possible source for wanting to do the bit magic, but I think that most targets would actually do better vectorizing a select at this point because vsel/bsl/blend type of instructions are usually part of a vector ISA. This came up in:
https://bugs.llvm.org/show_bug.cgi?id=6773#c9 (and I'm not locating the review thread, but Roman has proposed tests for the vector cases)

Can you provide an IR -> asm example for one/some of the problem cases that led you to this (or preferably, file bug reports for them)? If we can fix the cases that we know would regress with a reversal of this IR canonicalization, then we should push ahead with that effort and fix instcombine. We can't guarantee that there won't be regressions, but we should be able to fix problems as they are reported.


https://reviews.llvm.org/D45862