[PATCH] D82005: [InstCombine] Replace selects with Phis

[Sanjay Patel via Phabricator via llvm-commits]

spatel added subscribers: efriedma, reames, hfinkel.
spatel added a comment.

In D82005#2100372 <https://reviews.llvm.org/D82005#2100372>, @mkazantsev wrote:

> In D82005#2100058 <https://reviews.llvm.org/D82005#2100058>, @nikic wrote:
>
> > @mkazantsev I don't think this is about backend-specific optimizations (which indeed InstCombine should not be concerned about), phis and selects generally have different weak points in mid-level optimization. While you are right that the use of phis enables better jump threading, phis will usually not work with redundancy elimination and code motion optimizations. E.g. I don't think CSE or GVN do anything with phis right now.
> >
> > That said, I can definitely see the argument that //if// you already have a branch, it is beneficial to represent a select as a phi, and leave the decision on whether to turn the whole branch into a sequence of selects to EarlyCSE.
>
>
> Well, my patch doesn't break any existing test. I also have tests that get improved and may be potentially improved further. If you say that it might *not* be a profitable transform, please come up with a particular example.
>
> Actually, GVN does *a lot* about Phis (and mostly Phis) in PRE. And EarlyCSE does not walk above merge points anyways.


This discussion reminds me of:
https://bugs.llvm.org/show_bug.cgi?id=34603
cc @efriedma @hfinkel @reames

This patch converts an explicit use of the condition value into an implicit use via control-flow, so that seems like a good choice if we're early in optimization. Given that instcombine is used at every stage of the 'opt -Ox' pipeline though, I'm not sure what the overall implications will be.

So I don't object to this patch, but it would be good to hear from others if there are concerns.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82005/new/

https://reviews.llvm.org/D82005