[PATCH] D45862: [InstCombine] Remove decanonicalizing transforms of selects

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

In https://reviews.llvm.org/D45862#1081291, @spatel wrote:

> 1. I think it's clearly better for IR to eliminate the select if we can reduce the instruction count too.


Not if local improvement on one step will break another step which will break us even more improvement. You save one instruction by doing bit magic and kill the further transform that would eliminate 4 instructions. My point is that the canonicalization is generally a more useful thing in a long run than saving of one or two instructions now and pessimizing another more powerful transform later. Current strategy of saving one instruction by adding this bit magic produces horrible code on the tests I've added.

As for test15g, I will give some more thought to what can be done to it.

In https://reviews.llvm.org/D45862#1081291, @spatel wrote:

> So that's why I'm proposing to add optimizations for these patterns: https://rise4fun.com/Alive/XG7 in https://reviews.llvm.org/D46086 (note that test15g is not regressed there).


Why in InstCombine? These transforms are very simple, DAG selector should be able to do what you want, but later. The only possible profit I see from having this done in InstCombine is that it may affect the behavior of inlining/unrolling that calculate cost of the code, but it's only matter of tuning the cost functions if the need shows up.


https://reviews.llvm.org/D45862