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

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM



================
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.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68239