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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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);
----------------
goldstein.w.n wrote:
> nit style: Instead of nesting if statements, can you invert them and just return immediately? Think it makes code easier to read.
I've done this for the first check here and also for the isUnknown checks in https://github.com/llvm/llvm-project/commit/2d48a771fc00681414c54ea3936bff91f3b253c4.


================
Comment at: llvm/lib/Support/KnownBits.cpp:227
   // possible shifts.
   APInt MaxShiftAmount = RHS.getMaxValue();
+  if (!LHS.isUnknown() || (NUW && NSW)) {
----------------
goldstein.w.n wrote:
> in the NSW/NUW we should be able to further bound MaxShiftAmount with LHS.countLeadingZeros no? (and poison if MinShiftAmunt > LHS.countLeadingZeros)
Yes, but this is already implicitly handled by the loop below. It stops once it gets the first shift amount that results in poison due to nuw/nsw.


================
Comment at: llvm/lib/Support/KnownBits.cpp:250
   }
 
   Known.Zero.setLowBits(MinTrailingZeros);
----------------
goldstein.w.n wrote:
> 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.
You are right, it's necessary to check for conflicts here. I've added a general assertion for this in the exhaustive tests, so we don't have to remember checking it for every function: https://github.com/llvm/llvm-project/commit/d4bfc144eaa6e34eafaab1e8574421ebd2bb78ed


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

https://reviews.llvm.org/D151208