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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked an inline comment as done.
goldstein.w.n added inline comments.


================
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.
----------------
spatel wrote:
> 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
> 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