[PATCH] D142271: [ValueTracking] Add KnownBits patterns `xor(x, x - 1)` and `and(x, -x)` for knowing upper bits to be zero

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1087
+      if (match(I, m_c_BinOp(m_Value(X), m_Neg(m_Deferred(X))))) {
+        // -(-x) == x so pick whichever we can get a better result with.
+        if (Known.countMaxTrailingZeros() <= Known2.countMaxTrailingZeros())
----------------
foad wrote:
> A less "if"fy way to implement this would be:
> ```
>   Known = Known.blsi();
>   Known2 = Known2.blsi();
>   // should probably have a helper for the following two lines, similar to KnownBits::commonBits
>   Known.Zero |= Known2.Zero;
>   Known.One |= Known2.One;
> ```
> (But I'm still sceptical that it gives any practical benefit.)
> A less "if"fy way to implement this would be:
> ```
>   Known = Known.blsi();
>   Known2 = Known2.blsi();
>   // should probably have a helper for the following two lines, similar to KnownBits::commonBits
>   Known.Zero |= Known2.Zero;
>   Known.One |= Known2.One;
> ```

I'm not sure this is cleaner / clearer / faster.

> (But I'm still sceptical that it gives any practical benefit.)

Agreeds its not a big deal, but it can change the result and we don't need any extra expensive calls to handle it so seems like "why not" (would agree with you if we would need an extra `computeKnownBits` call or something). I added two new tests: `blsi_differing_lowbits` and `blsi_differing_lowbits2` which only work if we pick the minimum.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1096
+        else
+          Known = Known2.blsi();
+      }
----------------
nikic wrote:
> goldstein.w.n wrote:
> > nikic wrote:
> > > break here? Don't think we want to fall through.
> > > break here? Don't think we want to fall through.
> > 
> > Fallthrough here has no issue. While I think the `blsi` case is mutually exclusive with the other transform, I generally think breaking early in this type of code (where you have back-to-back cases) can make things unclear as the assumption is generally we will try them all.
> At least with this current code structure, falling through is pretty confusing.  `Known` was previously the known bits of the second operand (that is either X or -X) and now it will be the known bits of `X & -X`. I guess this is fine in practice because `&` is idempotent, so we end up computing something like `X & X & -X`.
> 
> The new code structure in D142427 avoids this problem, but for this patch I would not do the fall through.
> At least with this current code structure, falling through is pretty confusing.  `Known` was previously the known bits of the second operand (that is either X or -X) and now it will be the known bits of `X & -X`. I guess this is fine in practice because `&` is idempotent, so we end up computing something like `X & X & -X`.
> 
> The new code structure in D142427 avoids this problem, but for this patch I would not do the fall through.

Okay added the `break`, guess its only temporary anyways and doesn't change any of the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142271