[PATCH] D47428: [InstCombine] PR37603: low bit mask canonicalization

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

We didn't resolve the 'nuw' question - am I not seeing the scenario that you asked about?



================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1102
+/// Into:
+///   ~(-(1 << nbits))
+/// Because the `not` may end up being folded into `and`.
----------------
The comment should match what the code is doing:
~(-1 << NBits)


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1103
+///   ~(-(1 << nbits))
+/// Because the `not` may end up being folded into `and`.
+/// The new shl is always nsw. And is nuw if old `and` was.
----------------
The IR motivation is what I'd prefer to state here, so something like:
Because a 'not' is better for bit-tracking analysis and other transforms than an 'add'.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1112
+  Constant *MinusOne = Constant::getAllOnesValue(NBits->getType());
+  auto *Shl = cast<BinaryOperator>(Builder.CreateShl(MinusOne, NBits));
+  // Always NSW. But NUW propagates from `add`.
----------------
Is this always safe to cast rather than dyn_cast? What happens if NBits is a constant expression?


Repository:
  rL LLVM

https://reviews.llvm.org/D47428