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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
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,
----------------
spatel wrote:
> I don't see the value of the 'Check' lambda. Just do this check inline?
It will result in ugly cascade of if's - if one of preconditions does not match we can't just abort,
some next precondition can still match and thus allow the fold.
I believe it is better to keep lambda here.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66383