[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 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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144606