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

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev marked an inline comment as done.
anton-afanasyev added a comment.

Thanks to all, I've moved this fix to aggresive-instcombine, where it is even planned in `TODO:` section.



================
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
----------------
lebedev.ri wrote:
> 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.
> 
> To be noted, this is the profitability heuristics of instcombine: don't increase instruction count.
Ok, I see, thanks for noting this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107766