[PATCH] D131838: [ValueTracking] computeKnownBits - attempt to use a branch condition feeding a phi to improve known bits range

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1601
+
+              if (Pred == CmpInst::Predicate::ICMP_EQ)
+                Known2 = KnownBits::makeConstant(*RHSC);
----------------
Make this a switch with a TODO? Presumably, there's some inverted pred / Known.One sibling to these. We might also get more knowledge of signbits with signed preds.


================
Comment at: llvm/test/Transforms/InstCombine/known-phi-br.ll:35
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[CMP_NOT:%.*]] = icmp eq i64 [[X:%.*]], 255
 ; CHECK-NEXT:    br i1 [[CMP_NOT]], label [[END:%.*]], label [[BODY:%.*]]
----------------
This isn't testing the 'ne' pred (phi in the false block) that we wanted. Add another use to thwart that canonicalization? Or knownbits unit test for direct coverage?


================
Comment at: llvm/test/Transforms/InstCombine/known-phi-br.ll:57
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp ult i64 [[X:%.*]], 16
 ; CHECK-NEXT:    br i1 [[CMP]], label [[END:%.*]], label [[BODY:%.*]]
----------------
This isn't testing the 'ule' pred that we wanted. Add another use to thwart that canonicalization? Or knownbits unit test for direct coverage?


================
Comment at: llvm/test/Transforms/InstCombine/known-phi-br.ll:83
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp ugt i64 [[X:%.*]], 7
 ; CHECK-NEXT:    br i1 [[CMP]], label [[BODY:%.*]], label [[END:%.*]]
----------------
This isn't testing the 'uge' pred that we wanted. Add another use to thwart that canonicalization? Or knownbits unit test for direct coverage?


================
Comment at: llvm/test/Transforms/InstCombine/known-phi-br.ll:122
   %x.mask = phi i64 [ %x, %entry ], [ %mask, %body ]
   %res = and i64 %x.mask, 7
   ret i64 %res
----------------
Add a negative test just like this but with "x.mask & 3" to show that we didn't go overboard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131838