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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In D82005#2101927 <https://reviews.llvm.org/D82005#2101927>, @efriedma wrote:

> I think this makes sense as a canonicalization: if we could express a select idiom using either a PHI or a select, the PHI probably makes sense as the canonical form.  We clearly wouldn't want to canonicalize the other way.  (The more general discussion of select vs. control flow is more centered around whether we should have the control flow in the first place; that's outside the scope of instcombine.)
>
> The test coverage here seems a little slim; I'd like to see more coverage for interesting cases (in particular, cases where the PHI node doesn't have exactly two incoming edges).
>
> We could try to generalize this transform to try to insert the PHI in blocks other than the block that contains the select; maybe leave that as a followup, though.


I agree. We did discuss this general question in https://bugs.llvm.org/show_bug.cgi?id=34603#c10 and I think the conclusion is still valid: Assuming we don't cause regressions because of passes that don't handle control flow (and don't cause significant compile-time slowdowns or memory overhead), canonicalizing in favor of PHIs is where we should be aiming.


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

https://reviews.llvm.org/D82005