[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: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:
> lebedev.ri wrote:
> > 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.
> Why is using different variants based on bidwidth better? Or does -1 not work as a poisonous shift amount?
In this branch - yes, i think `-1` should work.
But in other branch we will negate that shift amount and add innermost shift bitwidth to it.
What i'm trying to say is, it is not so much about that the replacement shift amount *here*
will still produce `undef`, but it must do so when we actually perform the shift.
It seemed consistent to me to end up with same shift-by-bitwidth in both branches.

Let me know if that explanation does not make any sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68239