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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

I like this but there is a lot going on in the patch and I had to stop myself adding more comments....

Some of my comments aren't going to help reduce the patch as they're aren't directly related to the addition of the KnownBits, but you have quite a bit of refactoring (to APInt helper methods) in here already that should probably be done as NFC pre-commits.

I also don't think you need to include KnownBits.h in most places if you included it in ValueTracking.h ?



================
Comment at: include/llvm/Support/KnownBits.h:35
+  unsigned getBitWidth() const {
+    assert(Zero.getBitWidth() == One.getBitWidth());
+    return Zero.getBitWidth();
----------------
Missing assertion message


================
Comment at: lib/Analysis/DemandedBits.cpp:89
+        Known.Zero = APInt(BitWidth, 0);
+        Known.One = APInt(BitWidth, 0);
+        computeKnownBits(const_cast<Value *>(V1), Known, DL, 0,
----------------
Known = KnownBits(BitWidth) ? (and in other cases below)


================
Comment at: lib/Analysis/DemandedBits.cpp:95
+          Known2.Zero = APInt(BitWidth, 0);
+          Known2.One = APInt(BitWidth, 0);
+          computeKnownBits(const_cast<Value *>(V2), Known2, DL,
----------------
Known2 = KnownBits(BitWidth) ? (and in other cases)


================
Comment at: lib/Analysis/InstructionSimplify.cpp:3356
+      computeKnownBits(LHS, LHSKnown, Q.DL, /*Depth=*/0, Q.AC, Q.CxtI, Q.DT);
+      if (((LHSKnown.Zero & *RHSVal) != 0) || ((LHSKnown.One & ~(*RHSVal)) != 0))
         return Pred == ICmpInst::ICMP_EQ ? ConstantInt::getFalse(ITy)
----------------
APInt::intersects?


================
Comment at: lib/Analysis/InstructionSimplify.cpp:4722
+    computeKnownBits(I, Known, DL, /*Depth*/0, AC, I, DT, ORE);
+    if ((Known.Zero | Known.One).isAllOnesValue())
+      Result = ConstantInt::get(I->getType(), Known.One);
----------------
This might be worth a convenience method as we often want to determine if KnownBits covers all bits.


================
Comment at: lib/Analysis/ValueTracking.cpp:817
+    // bits so that other code could propagate this undef.
+    if ((Known.Zero & Known.One) != 0) {
+      Known.Zero.clearAllBits();
----------------
APInt::intersects


================
Comment at: lib/Analysis/ValueTracking.cpp:2015
+
+    auto OppositeBits = (Known1.Zero & Known2.One) | (Known2.Zero & Known1.One);
     if (OppositeBits.getBoolValue())
----------------
APInt?


================
Comment at: lib/Analysis/ValueTracking.cpp:4264
 
-        if ((KnownZero & *CA) == *CA && (KnownZero & *CB) == *CB)
+        if ((Known.Zero & *CA) == *CA && (Known.Zero & *CB) == *CB)
           return true;
----------------
APInt::isSubsetOf ?


================
Comment at: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:115
+      Known.getBitWidth() == BitWidth &&
       "Value *V, DemandedMask, KnownZero and KnownOne "
       "must have same BitWidth");
----------------
Update message - still refers to KnownZero and KnownOne


================
Comment at: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:141
   if (Depth != 0 && !I->hasOneUse()) {
-    return SimplifyMultipleUseDemandedBits(I, DemandedMask, KnownZero, KnownOne,
-                                           Depth, CxtI);
+    return SimplifyMultipleUseDemandedBits(I, DemandedMask, Known, Depth, CxtI);
   }
----------------
braces


================
Comment at: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:199
+    assert(!(RHSKnown.Zero & RHSKnown.One) && "Bits known to be one AND zero?");
+    assert(!(LHSKnown.Zero & LHSKnown.One) && "Bits known to be one AND zero?");
 
----------------
APInt::intersects


================
Comment at: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:231
+    assert(!(RHSKnown.Zero & RHSKnown.One) && "Bits known to be one AND zero?");
+    assert(!(LHSKnown.Zero & LHSKnown.One) && "Bits known to be one AND zero?");
 
----------------
APInt::intersects


================
Comment at: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:320
+    assert(!(RHSKnown.Zero & RHSKnown.One) && "Bits known to be one AND zero?");
+    assert(!(LHSKnown.Zero & LHSKnown.One) && "Bits known to be one AND zero?");
 
----------------
APInt::intersects


================
Comment at: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:403
+    Known.One = Known.One.zext(BitWidth);
+    assert(!(Known.Zero & Known.One) && "Bits known to be one AND zero?");
 
----------------
APInt::intersects


================
Comment at: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:547
 
-      assert(!(KnownZero & KnownOne) && "Bits known to be one AND zero?");
+      assert(!(Known.Zero & Known.One) && "Bits known to be one AND zero?");
       // Compute the new bits that are at the top now.
----------------
APInt::intersects


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2884
+      computeKnownBits(I, Known, /*Depth*/0, I);
+      if ((Known.Zero | Known.One).isAllOnesValue()) {
+        Constant *C = ConstantInt::get(Ty, Known.One);
----------------
KnownBits helper


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4384
     APInt CaseVal = Case.getCaseValue()->getValue();
-    if ((CaseVal & KnownZero) != 0 || (CaseVal & KnownOne) != KnownOne ||
+    if ((CaseVal & Known.Zero) != 0 || (CaseVal & Known.One) != Known.One ||
         (CaseVal.getMinSignedBits() > MaxSignificantBitsInCond)) {
----------------
APInt::intersects and APInt::isSubsetOf


https://reviews.llvm.org/D32376