[PATCH] D68239: [InstCombine] dropRedundantMaskingOfLeftShiftInput(): propagate undef shift amounts

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:117-126
+  auto Fix = [&](Constant *V) {
+    return !V || !isa<UndefValue>(V) ? V : Replacement;
+  };
+
+  if (auto *CV = dyn_cast<ConstantVector>(C)) {
+    llvm::SmallVector<Constant *, 32> NewOps(CV->getNumOperands());
+    for (const auto &I : llvm::zip(CV->operands(), NewOps))
----------------
I suppose it's a matter of C++ familiarity, but this logic is not obvious to me.

I think it's easier to read as something like:
  unsigned NumElts = CV->getType()->getVectorNumElements();
  for (unsigned i = 0; i != NumElts; ++i) {
    Constant *EltC= CV->getOperand(i);
    NewOps[i] = match(EltC, m_Undef()) ? Replacement : EltC;
  }


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:190-192
+      // If any of these shift amounts are undef, *ext will turn them into
+      // zeros, let's keep undef's by replacing them with some illegal shift
+      // amount.
----------------
Probably worth explaining this more directly, and what if we make it consistent by replacing with -1?
  // An extend of an undef value becomes zero because the 
  // high bits are never completely unknown. Replace the
  // the shift amount with -1 to ensure that the value remains
  // undef when creating the subsequent shift op.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68239