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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:116
 
+static Constant *replaceUndefsWith(Constant *C, Constant *Replacement) {
+  auto Fix = [&](Constant *V) {
----------------
spatel wrote:
> Is it possible to get here with a scalar undef? If so, this doesn't replace it. If not, do we want to assert that we have a vector constant?
Given that we get the constant via running a custom instsimplify
i wouldn't fully exclude the situation that we'd  end with scalar `undef` here.
It's not *really* a correctness problem.
Let's not assert but also replace.


================
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.
----------------
spatel wrote:
> 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.
Good comments are always good :)
Not with -1 though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68239