[PATCH] D133362: [InstCombine] Fold x + (x | -x) to x & (x - 1)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/add_or_sub.ll:154
   %or = or i10 %sub, %x
   %add = add nsw nuw i10 %or, %x
   ret i10 %add
----------------
spatel wrote:
> marcauberer wrote:
> > spatel wrote:
> > > Can we propagate the no-wrap flags to the new add (ie, is it correct based on Alive2 proofs)?
> > Do we need to propagate the no-wrap flags?
> > 
> > Without propagating the flags: https://alive2.llvm.org/ce/z/3rM4KF
> > With propagating the flags: https://alive2.llvm.org/ce/z/tWyFm7
> If we can propagate/generate flags during the transform, that is best. It saves some potentially expensive (in compile-time) analysis that tries to regenerate the flags or prevents losing information completely.
> 
> Note that "nuw" on these patterns is a special-case:
> https://alive2.llvm.org/ce/z/h1763a
> 
> I have no idea if that pattern occurs in the real world, but we don't seem to get the optimal result with or without this patch.
> 
> It's a small change to the Builder.CreateAdd() call to propagate flags from "I", so we should do that. Please add tests with "nsw" alone and "nuw" alone to verify that we do the right thing in all cases.
On 2nd thought, just add no-wrap flags to some of the adds in the existing tests, and that will more efficiently verify that we are behaving correctly. We don't really need explicit tests just to show flag propagation. You can add a code comment above the test or modify the test function name to explain the intent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133362