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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:864
+
+  // Check that the bitwidth of the operation is a power of two, and that
+  // the shift amount is 1/2 of it.
----------------
lebedev.ri wrote:
> Is it not sufficient to just check that the width is 2x the shift amount?
> Do we need the power-of-two check?
I don't think the power of 2 check is really needed - I inherited it from the original patch. I will remove it


================
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);
----------------
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?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:917-918
+  // Replace the use of `Op0` by `I` with `Overflow`.
+  if (IsAShr)
+    return new SExtInst(Overflow, Ty);
+  return new ZExtInst(Overflow, Ty);
----------------
lebedev.ri wrote:
> `mul i16 255, 2` is `i16 510`, which is non-negative,
> so the signbit of the original result is never set.
> IOW, just zero-extend.
Is it normal that Alive2 reports the transformation as incorrect for i2 then (see @foad's comment above) ?
Maybe we just need to SExt if it's i2 + ashr?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138814