[PATCH] D144606: [InstCombine] Add transforms for `(max/min (xor X, Pow2), X)` -> `(and/or X, Pow2/~Pow2)`

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1498
+          Type *Ty = I0->getType();
+          // Negative power of 2 must be IntMin. Its possible to be able to
+          // prove negative / power of 2 without actually having known bits, so
----------------
spatel wrote:
> goldstein.w.n wrote:
> > spatel wrote:
> > > If we can prove that some value is a constant, then shouldn't we have zapped it already?
> > > The test example shows that we are missing a fold like this:
> > > https://alive2.llvm.org/ce/z/z5xok2
> > > If we can prove that some value is a constant, then shouldn't we have zapped it already?
> > > The test example shows that we are missing a fold like this:
> > > https://alive2.llvm.org/ce/z/z5xok2
> > 
> > It's one of the cases that falls through the cracks. `IsPow2 && Negative` implies int-min but we don't have an explicit check for that anywhere.
> > 
> > I considered adding it in `computeKnownBits` but it seems like an edge case that is not worth the significant amounts of extra analysis needed to cover it.
> > (similiar to how `isKnownPowerOf2(X, /*OrZero*/false)` misses alot of cases that could be covered if we just did `isKnownPowerOf2(X, /*OrZero*/true) && isKnownNonZero(X)` and `isKnownNonZero(X)` misses some cases that `computeKnownBits(X)` hits)
> > 
> > ValueTracking imo is a bit of a mess at the moment, but most of the easy fixes have compile time concerns.
> Yes, it's messy, and it's not clear what is worth generalizing vs. pattern matching directly.
> 
> I filed https://github.com/llvm/llvm-project/issues/60957 - if you're seeing BMI-related patterns like this in real code, then it's probably worth getting that one way or another.
Its -> It's


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1498-1500
+          // Negative power of 2 must be IntMin. Its possible to be able to
+          // prove negative / power of 2 without actually having known bits, so
+          // just get the value by hand.
----------------
goldstein.w.n wrote:
> spatel wrote:
> > If we can prove that some value is a constant, then shouldn't we have zapped it already?
> > The test example shows that we are missing a fold like this:
> > https://alive2.llvm.org/ce/z/z5xok2
> > If we can prove that some value is a constant, then shouldn't we have zapped it already?
> > The test example shows that we are missing a fold like this:
> > https://alive2.llvm.org/ce/z/z5xok2
> 
> It's one of the cases that falls through the cracks. `IsPow2 && Negative` implies int-min but we don't have an explicit check for that anywhere.
> 
> I considered adding it in `computeKnownBits` but it seems like an edge case that is not worth the significant amounts of extra analysis needed to cover it.
> (similiar to how `isKnownPowerOf2(X, /*OrZero*/false)` misses alot of cases that could be covered if we just did `isKnownPowerOf2(X, /*OrZero*/true) && isKnownNonZero(X)` and `isKnownNonZero(X)` misses some cases that `computeKnownBits(X)` hits)
> 
> ValueTracking imo is a bit of a mess at the moment, but most of the easy fixes have compile time concerns.
Yes, it's messy, and it's not clear what is worth generalizing vs. pattern matching directly.

I filed https://github.com/llvm/llvm-project/issues/60957 - if you're seeing BMI-related patterns like this in real code, then it's probably worth getting that one way or another.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144606