[PATCH] D66383: [InstCombine] Shift amount reassociation in bittest: trunc-of-lshr (PR42399)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

A few clean-ups noted inline.

If I'm reading it correctly, this is more complicated than it could be only to support arbitrary vector constants. Do we have any evidence that says we need that support?

We've come this far on this series of patches without raising that question, so I'm not going to object to this particular patch now. But I think we should keep the code simpler unless we know there's a reason to handle the arbitrary vector constant pattern. It seems too rare to me to warrant this much effort.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3389
+  if (HadTrunc && match(WidestShift, m_LShr(m_Value(), m_Value()))) {
+    auto Check = [SQ, WidestShift, NarrowestShift, NewShAmt, WidestTy]() {
+      auto CstMatch = [](Value *V, ICmpInst::Predicate Predicate,
----------------
I don't see the value of the 'Check' lambda. Just do this check inline?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3390
+    auto Check = [SQ, WidestShift, NarrowestShift, NewShAmt, WidestTy]() {
+      auto CstMatch = [](Value *V, ICmpInst::Predicate Predicate,
+                         uint64_t Val) {
----------------
"Cst" is ambiguous (I always read that as "Cast"). "MatchCmpInteger"?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3396
+      };
+      unsigned WidestBitWidth = WidestTy->getScalarSizeInBits();
+
----------------
Move this local variable declaration/assignment up, so it can be used in lines 3383/3384?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3422
+      // We consider *min* leading zeros so a single outlier
+      // blocks the the transform as opposed to allowing it.
+      // FIXME: this is pessimistic for vectors.
----------------
typo: 'the the'


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3425
+
+      if (auto C = dyn_cast<Constant>(NarrowestShift->getOperand(0))) {
+        // Precondition:  NewShAmt u<= countLeadingZeros(C)
----------------
We prefer "auto *" based on current guidelines:
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3431
+      }
+      if (auto C = dyn_cast<Constant>(WidestShift->getOperand(0))) {
+        // Precondition:  (63-NewShAmt) u<= countLeadingZeros(C)
----------------
We prefer "auto *" based on current guidelines.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3432
+      if (auto C = dyn_cast<Constant>(WidestShift->getOperand(0))) {
+        // Precondition:  (63-NewShAmt) u<= countLeadingZeros(C)
+        KnownBits K = computeKnownBits(C, SQ.DL);
----------------
xbolva00 wrote:
> Replace 63 with WidestBitWidth - 1?
Generalize "63" to "WideWidth - 1"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66383