[PATCH] D138814: [InstCombine] Combine a/lshr of add -> uadd.with.overflow

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:880-891
+  // Make sure that `Op0` is only used by `I` and `HalfWidth`-truncates.
+  SmallVector<TruncInst *, 2> Truncs;
+  for (User *Usr : Op0->users()) {
+    if (Usr == &I)
+      continue;
+
+    TruncInst *Trunc = dyn_cast<TruncInst>(Usr);
----------------
Pierre-vh wrote:
> Pierre-vh wrote:
> > lebedev.ri wrote:
> > > This is rather unusual in InstCombine.
> > > Would it not be sufficient to just zext the result?
> > I think the original intent was to only to do the transform when it appears beneficial, but if we're fine with doing it all the time as a canonical form then we can indeed just zext + RAUW. What do you think?
> Now that I think about it again, I don't think the transform would be valid if we don't look for the truncs?
> We would be replacing the .add - which may have <32 leading zeroes (if overflow), with a zext of the uaddo, which will always have 32 leading zeroes or more
Ah yes, you look at truncs of the wide add.
It wouldn't be illegal to just leave it be.

I think what you want here, is:
```
if(!Op0.hasOneUse()) {
  for (User *Usr : Op0->users()) {
    if (Usr == &I)
      continue;

    TruncInst *Trunc = dyn_cast<TruncInst>(Usr);
    if (!Trunc || Trunc->getType()->getScalarSizeInBits() > HalfWidth)
      return nullptr;
  }
}
```

and then just
```
if(!Op0.hasOneUse()) {
   Value*WideOV = Builder.CreateZExt(Overflow);
   replaceInstUsesWith(Op0, WideOV);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138814