[PATCH] D148414: [InstCombine] Expand `foldSelectICmpAndOr` -> `foldSelectICmpAndBinOp` to work for any binop

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

In D148414#4290296 <https://reviews.llvm.org/D148414#4290296>, @nikic wrote:

> Why is this legal for all binops? Doesn't this transform require that the neutral element of the binop is zero? So it would work for or, xor or add, but not for mul or and for example?

Yeah you are right. Not sure what I was thinking...

> I think this entire transform should probably be handled as a two step process: First, `Cond ? X : BinOp(X, C)` should become `BinOp(X, Cond ? NeutralC : C)` and then this fold should work on `Cond ? 0 : C` as the root.
> We actually already do the former canonicalization, but not in the case where `C` is constant.

Where do we do the canonicalization?

> This will cleanly separate out the actual binop handling. Only disadvantage is that we won't be able to handle the case where the binop has multiple uses anymore, but that's a general issue with composable folds that we can ignore unless there is reason to believe that multi-use is motivating for the current handling.

Why won't we be able to handle multi-use anymore? If we do it like this, seems this function can be deleted once we get the canonicalization of `Cond ? X : BinOp(X, C)` -> `BinOp(X, Cond ? NeutralC : C)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148414