[PATCH] D107766: [InstCombine] Get rid of `hasOneUses()` when swapping `lshr` and `zext`

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

FWIW i agree that it obviously improves the SLP snippet in question,
i'm just not sure this is the right way to do it. Sorry.



================
Comment at: llvm/test/Transforms/InstCombine/pr50555.ll:9
+; CHECK-NEXT:    [[SHR:%.*]] = zext i8 [[TMP1]] to i32
+; CHECK-NEXT:    [[ADD:%.*]] = add nuw nsw i32 [[ZEXT]], [[SHR]]
 ; CHECK-NEXT:    store i32 [[ADD]], i32* [[A:%.*]], align 4
----------------
anton-afanasyev wrote:
> >>! In D107766#2934536, @lebedev.ri wrote:
> > We can not do this transform as proposed here,
> > it increases the instruction count.
> > 
> > Could you point me at the test for this change?
> > It should only contain lshr-of-zext, there should not be any trunc;
> > please add a test where zext has an extra use.
> 
> @lebedev.ri: Yes, you're right, it increases instruction count, adding new `zext` when the old one  has an extra use. But it also simplifies `lshr` to lower bits type making it simpler. Isn't it a good compromise?
> 
> And also, after `zext` sinking, it can trigger a chain of changes combining `zext` with other instrs like in cases below: `add`, `trunc` and so on.
> @lebedev.ri: Yes, you're right, it increases instruction count, adding new zext when the old one has an extra use. 
To be noted, this is the profitability heuristics of instcombine: don't increase instruction count.

> But it also simplifies lshr to lower bits type making it simpler. Isn't it a good compromise?
> And also, after zext sinking, it can trigger a chain of changes combining zext with other instrs like in cases below: add, trunc and so on.

Sure. But it still increases instruction count.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107766