[PATCH] D107552: [InstCombine] Combine lshr of add that intends to get the carry as llvm.uadd.with.overflow

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:1105
+    for (TruncInst *Trunc : Truncs) {
+      Trunc->replaceAllUsesWith(UAdd);
+    }
----------------
lebedev.ri wrote:
> craig.topper wrote:
> > craig.topper wrote:
> > > We probably need to do something here to schedule the dead truncates to revisited to remove them. But I'm not sure.
> > I think this should use InstCombinerImpl::replaceInstUsesWith rather than Instruction::replaceAllUsesWith. Then I think it should call InstCombinerImpl::eraseInstFromFunction.
> > 
> > @spatel or @lebedev.ri does that sound right?
> Right, the pass-specific call should be used.
> I don't think we need to call `eraseInstFromFunction()` explcitly,
> that should be handled automatically once you return non-null from this function.
> Generally, i would think not having an intermediate vector would be better, but not sure.
The truncates aren’t directly part of the chain of instructions we’re changing so I’m not sure if the get queued properly for DCE once we return.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107552