[PATCH] D151208: [KnownBits] Add support for nuw/nsw on shifts

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Support/KnownBits.cpp:176
+    Known.One = LHS.One << ShiftAmt;
+    if ((NUW || NSW) && ShiftAmt != 0) {
+      KnownBits ShiftedOutBits = LHS.extractBits(ShiftAmt, BitWidth - ShiftAmt);
----------------
nit style: Instead of nesting if statements, can you invert them and just return immediately? Think it makes code easier to read.


================
Comment at: llvm/lib/Support/KnownBits.cpp:215
   APInt MinShiftAmount = RHS.getMinValue();
   if (MinShiftAmount.uge(BitWidth)) {
     // Always poison. Return zero because we don't like returning conflict.
----------------
Imo, this check should just be at the top. Then you can drop the `RHS.getConstant().ult(BitWidth)` check before `ShiftByConst`


================
Comment at: llvm/lib/Support/KnownBits.cpp:227
   // possible shifts.
   APInt MaxShiftAmount = RHS.getMaxValue();
+  if (!LHS.isUnknown() || (NUW && NSW)) {
----------------
in the NSW/NUW we should be able to further bound MaxShiftAmount with LHS.countLeadingZeros no? (and poison if MinShiftAmunt > LHS.countLeadingZeros)


================
Comment at: llvm/lib/Support/KnownBits.cpp:250
   }
 
   Known.Zero.setLowBits(MinTrailingZeros);
----------------
can you assert no conflict? Its not super obvious just reading the code that the loop will be enough to clear the all zeros/ones initial state in Known.


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

https://reviews.llvm.org/D151208