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

Mon Apr 24 02:08:10 PDT 2023

nikic added a comment.

In D148414#4291480 <https://reviews.llvm.org/D148414#4291480>, @goldstein.w.n wrote:

> In D148414#4290852 <https://reviews.llvm.org/D148414#4290852>, @nikic wrote:
>
>> In D148414#4290790 <https://reviews.llvm.org/D148414#4290790>, @goldstein.w.n wrote:
>>
>>> 2. Update `foldSelectIntoOp` to do constants iff `isPow2(abs(TC - FC))` and `Cond` is  either `(ICmp eq Pow2OrZero, C_Pow2)` or a SignTest
>>
>> Ideally we would just remove the limitation entirely -- at the IR level, we would certainly prefer having a select on constants. However, this would likely need a backend undo transform, because that saves materializing the zero constant and likely allows folding the other constant into an immediate operand.
>
> @nikic so the backend has code to undo this already. Its just not enabled for scalars. I enabled it on a branch for x86 here:
> https://github.com/goldsteinn/llvm-project/pull/new/enable-select-backend        
> but a lot of regressions. Maybe simpler to just handle the known cases here?

I think your patch handles a few more cases than we are interested in here. Per the code in getSelectFoldableOperands() we don't do this transform for div/rem, so I don't think we need the additional handling for those. More importantly, it looks like your patch will also do the undo transform for the case where the select has the identity as one element and a non-constant as the other. I think the diffs will look better if you limited the scalar case to two constant operands. The constant + non-constant case can probably also be beneficial, but the heuristics for that are less obvious.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148414