[PATCH] D91343: [InstCombine] Optimize away the unnecessary multi-use sign-extend

[Bhramar Vatsa via Phabricator via llvm-commits]

Bhramar.vatsa added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:922
     // Compute the Known bits to simplify things downstream.
     computeKnownBits(I, Known, Depth, CxtI);
 
----------------
lebedev.ri wrote:
> craig.topper wrote:
> > lebedev.ri wrote:
> > > Bhramar.vatsa wrote:
> > > > For this patch, this call will no more happen, even when the 'case AShr' doesn't match completely. So, should a call to computeKnownBits(), be added in that case?
> > > > The call isn't there for other cases, but there, perhaps it isn't needed.
> > > Yes, this doesn't look immediately correct to me, but it's a preexisting problem.
> > > I think the contents of `default:` switch should be located after the switch.
> > > I think it would be an interesting experiment to make that change,
> > > but not in this review.
> > Don't the other handlers already do the equivalent of the default?
> The equivalent being computing the `Known` - err, right, they do.
So, then, perhaps in the favor of not changing the functionality when optimization doesn't take place, should the code for the default case be added for 'AShr' case too?
If agreed, I will provide another patch with this change.


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

https://reviews.llvm.org/D91343