[PATCH] D64512: [InstCombine] Dropping redundant masking before left-shift [0/5] (PR42563)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a subscriber: efriedma.
spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:81-83
+  Value *Masked, *ShiftShAmt;
+  if (!match(OuterShift, m_Shl(m_Value(Masked), m_Value(ShiftShAmt))))
+    return nullptr;
----------------
assert on opcode and assign operands rather than using match - the input must be 'shl'?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:103-104
+  unsigned BitWidth = X->getType()->getScalarSizeInBits();
+  if (!match(SumOfShAmts, m_SpecificInt_ICMP(ICmpInst::Predicate::ICMP_UGE,
+                                             APInt(BitWidth, BitWidth))))
+    return nullptr;
----------------
lebedev.ri wrote:
> It may be possible to use `SimplifyICmpInst()` here, to catch more cases, may look into that after patchset.
Using instsimplify like that (or as currently in this patch with SimplifyBinOp) is where we go into the InstCombine gray area of, "Is it worth the cost?"

With InstSimplify, we're using a potentially expensive/recursive analysis (similar to ValueTracking), and we know that's where InstCombine spends a lot of time. 

cc'ing @efriedma for further thoughts.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:108
+
+  // No 'NUW'/'NSW'!
+  return BinaryOperator::Create(OuterShift->getOpcode(), X, ShiftShAmt);
----------------
Add a code comment similar to what you explained here in the review:
"We no longer know that we won't shift-out non-0 bits."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64512