[PATCH] D138814: [InstCombine] Combine a/lshr of add -> uadd.with.overflow
Pierre van Houtryve via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 30 05:14:44 PST 2022
Pierre-vh 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);
----------------
lebedev.ri wrote:
> 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);
> }
> ```
Indeed that works too, but I think you meant `CreateZExt(UAdd)` instead of the Overflow bit?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138814/new/
https://reviews.llvm.org/D138814
More information about the llvm-commits
mailing list