[PATCH] D104193: [InstCombine] Require one user (not one use) for operands when optimize both (sext bool X) * (sext bool Y) and (zext bool X) * (zext bool Y)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D104193#2815649 <https://reviews.llvm.org/D104193#2815649>, @vdsered wrote:

> In D104193#2815644 <https://reviews.llvm.org/D104193#2815644>, @lebedev.ri wrote:
>
>> Correction: what i meant to say is, there are two cases:
>>
>> 1. `X == Y`, in which case we do not care about use count. this implicitly handles the case where both operands are the same instruction
>> 2. either operand is one-use
>
> I'd add one more case where one operand has one use and an other more than one. We check this for zext in mul_bools_use2 and for sext in mul_bools_sext_use1 and mul_bools_sext_use2 (they seem to differ only in operand's order, should it be the same for zext?).
>
> Regarding your cases...
>
> I can be wrong, but we do care about use count when X == Y because the way how we optimize depends on it. Look at the example below
>
>   declare void @use(i32)
>   define i32 @src(i1 %x, i1 %y) {
>     %zx = zext i1 %x to i32
>     %r = mul i32 %zx, %zx
>     call void @use(i32 %zx)
>     ret i32 %r
>   }
>
> If there is more than one user (or 2 uses in other words), we cannot replace
>
>   %zx = zext i1 %x to i32
>   %r = mul i32 %zx, %zx
>
> by
>
>   %r = and i1 %x, %y
>   %zy = zext i1 %r to i32
>
> because zx has another user (call of @use(i32)). Otherwise, we need to leave zx as-is. I'm not sure if we should do this (would it be beneficial?). I didn't intend to handle this particular case as a part of that patch.

I'm not sure i follow.
`%r = and i1 %x, %x` is just `%x`, is it not?
So you end up with a single `zext`.

> The second case where either of ops has one use. Yes, it must be legal to do such a transformation when both ops has one use and they are different instruction. As an addition alive says it is correct <https://alive2.llvm.org/ce/z/ADMksb>. Previously, it was Op0->hasOneUse() || Op1->hasOneUse() and when there is one use, then there is one user so it must be equivalent to I.isOnlyUserOfAnyOperand() in that case.
>
> I guess, tests like mul_bools_sext_use2 must partially check the second case, but yes, there are no tests like.
>
>   define i32 @func(i1 %x, i1 %y) {
>     %sx = sext i1 %x to i32
>     %sy = sext i1 %y to i32
>     %r = mul i32 %sy, %sx
>     ret i32 %r
>
> I'll update the diff




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104193