This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Allow values with multiple users in SimplifyDemandedVectorElts
ClosedPublic

Authored by piotr on Sep 9 2019, 3:46 AM.

Details

Summary

Allow for ignoring the check for a single use in SimplifyDemandedVectorElts
to be able to simplify operands if DemandedElts is known to contain
the union of elements used by all users.
It is a responsibility of a caller of SimplifyDemandedVectorElts to
supply correct DemandedElts.

Simplify a series of extractelement instructions if only a subset of
elements is used.

Diff Detail

Event Timeline

piotr created this revision.Sep 9 2019, 3:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2019, 3:46 AM
arsenm added inline comments.Sep 9 2019, 8:17 AM
lib/Transforms/InstCombine/InstCombineVectorOps.cpp
294 ↗(On Diff #219309)

dyn_cast_or_null

arsenm added inline comments.Sep 9 2019, 8:21 AM
lib/Transforms/InstCombine/InstCombineVectorOps.cpp
294 ↗(On Diff #219309)

Nevermind, this isn't quite that

This is a useful change, but there is an unfortunate asymmetry here in how the code is structured: in addition to extractelement, we could also have shufflevector users (or masked stores etc.). Presumably we'd be able to handle all of those together without duplicating the code. Is there a way to take this into account?

Also, two comments inline.

lib/Transforms/InstCombine/InstCombineVectorOps.cpp
296 ↗(On Diff #219309)

ult instead of ule.

304–305 ↗(On Diff #219309)

Skip this if DemandedElts is allOnes.

piotr added a comment.Oct 1 2019, 5:02 AM

This is a useful change, but there is an unfortunate asymmetry here in how the code is structured: in addition to extractelement, we could also have shufflevector users (or masked stores etc.). Presumably we'd be able to handle all of those together without duplicating the code. Is there a way to take this into account?

Also, two comments inline.

Good point, I will try extending the patch to also handle shufflevector.

piotr updated this revision to Diff 225165.Oct 16 2019, 12:04 AM

As suggested by Nicolai, I extended the patch to also handle shufflevector users in addition to extractelement. The new generalized version also simplifies the case where three extract elements are used (the previous patch only handled two). The new code also makes it easy to extend the case list even more in the future.

This revision is now accepted and ready to land.Oct 18 2019, 2:39 AM
This revision was automatically updated to reflect the committed changes.