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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked 4 inline comments as done.
lebedev.ri added a comment.

In D68239#1696589 <https://reviews.llvm.org/D68239#1696589>, @spatel wrote:

> LGTM


Thank you for the review!

There's second half of the patch in D68470 <https://reviews.llvm.org/D68470>.
I did't post the actual patch adding trunc support yet, tests needed...



================
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:
> > > 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.
> Ok, I see what you mean. It seems like we're missing some analysis that would handle this sort of thing for us cleanly, but I don't see where/how to implement it, so I won't hold this up.
Yeah..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68239