[PATCH] D32376: [ValueTracking] Introduce a KnownBits struct to wrap the two APInts for computeKnownBits

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Analysis/ValueTracking.cpp:1399
+        Known.Zero |= Known2.Zero.reverseBits();
+        Known.One |= Known2.One.reverseBits();
         break;
----------------
RKSimon wrote:
> Not due to this patch but shouldn't this be:
> ```
> Known.Zero = Known2.Zero.reverseBits();
> Known.One = Known2.One.reverseBits();
> ```
I believe I added the OR so that we would preserve bits computed from range metadata earlier.

But if you think that's incorrect we can certainly change it back and add a comment that it was intentionally different from the other intrinsics.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:179
 /// known one bits, returning them in Min/Max.
-static void computeSignedMinMaxValuesFromKnownBits(const APInt &KnownZero,
-                                                   const APInt &KnownOne,
+static void computeSignedMinMaxValuesFromKnownBits(const KnownBits &Known,
                                                    APInt &Min, APInt &Max) {
----------------
RKSimon wrote:
> computeSignedMinMaxValuesFromKnownBits and computeUnsignedMinMaxValuesFromKnownBits might be useful inside KnowBits someday if we need them in SelectionDAG as well? Worth a TODO?
I'll add the TODO


https://reviews.llvm.org/D32376