[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:869-871
+  if (computeKnownBits(X, 0, &I).countMinLeadingZeros() != NumLeadingZeroes ||
+      computeKnownBits(Y, 0, &I).countMinLeadingZeros() != NumLeadingZeroes)
+    return nullptr;
----------------
What we want to know here, is whether both `X` and `Y` can be `NUW`-truncated to `ShAmt`-wide types.
This is award because we are tiptoeing around adding new QoL helper functions.
We already have this: (ValueTracking.h/cpp)
```
/// Get the upper bound on bit size for this Value \p Op as a signed integer.
/// i.e.  x == sext(trunc(x to MaxSignificantBits) to bitwidth(x)).
/// Similar to the APInt::getSignificantBits function.
unsigned ComputeMaxSignificantBits(const Value *Op, const DataLayout &DL,
                                   unsigned Depth = 0,
                                   AssumptionCache *AC = nullptr,
                                   const Instruction *CxtI = nullptr,
                                   const DominatorTree *DT = nullptr);
``` 
but that is for sign bits, while we want zero bits.
Can you just add an unsigned variant of it next to it, and use it here?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:908
+
+  Builder.SetInsertPoint(&(*RestoreInsPt));
+
----------------
I think we should just emit it all next to the original `add`,
there isn't much point in sinking the final zext.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:902-905
+  // Replace the uses of the original add with a zext of the
+  // uaddo's result.
+  if (!Add->hasOneUse())
+    replaceInstUsesWith(*AddInst, Builder.CreateZExt(UAdd, Ty));
----------------
Pierre-vh wrote:
> lebedev.ri wrote:
> > alive2 proof please? 
> At this stage, the add is only used by either ShAmt-sized truncs, or the shift.
> We're removing the shift, and for the truncs, they will cancel out.
> https://alive2.llvm.org/ce/z/TWQp29
Right, i'm being slow. This is correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138814