This is an archive of the discontinued LLVM Phabricator instance.

[X86][SSE] Avoid specifying unused arguments in SHUFPD lowering
ClosedPublic

Authored by RKSimon on Aug 1 2016, 12:12 PM.

Details

Summary

As discussed on PR26491, we are missing the opportunity to make use of the smaller MOVHLPS instruction because we set both arguments of a SHUFPD when using it to lower a single input shuffle.

This patch sets the lowered argument to UNDEF if that shuffle element is undefined. This in turn makes it easier for target shuffle combining to decode UNDEF shuffle elements, allowing combines to MOVHLPS to occur.

A fix to match against MOVHPD stores was necessary as well.

This builds on the improved MOVLHPS/MOVHLPS lowering and memory folding support added in D16956

Adding similar support SHUFPS will have to wait until have better support for target combining of binary shuffles.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 66353.Aug 1 2016, 12:12 PM
RKSimon retitled this revision from to [X86][SSE] Avoid specifying unused arguments in SHUFPD lowering.
RKSimon updated this object.
RKSimon added reviewers: ab, delena, spatel, andreadb.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: llvm-commits.
spatel accepted this revision.Aug 20 2016, 1:33 PM
spatel edited edge metadata.

LGTM. See inline comment for one question.

lib/Target/X86/X86ISelLowering.cpp
8965–8966 ↗(On Diff #66353)

I can never keep this straight, but shouldn't we use '== SM_SentinelUndef' here instead of '< 0'?

Ie, we get here from lowerVectorShuffle() which uses ShuffleVectorSDNode->getMask() which says "An index of -1 is treated as undef..."

This revision is now accepted and ready to land.Aug 20 2016, 1:33 PM
craig.topper added inline comments.
lib/Target/X86/X86ISelLowering.cpp
8965–8966 ↗(On Diff #66353)

If this comes from a generic VECTOR_SHUFFLE, then we can assume that the only value less than 0 will be -1 so the < 0 is safe. The SM_SentinelUndef is needed if you can get here using a mask from a target shuffle which uses -2 to represent zeroable. Sometimes I think we should use a zeroable bit vector for target shuffles like we do for VECTOR_SHUFFLE.

RKSimon added inline comments.Aug 22 2016, 5:42 AM
lib/Target/X86/X86ISelLowering.cpp
8965–8966 ↗(On Diff #66353)

We do actually now assert in lowerVectorShuffle that we only use -1 for undefined (and permit no other -ve values) so we can safely use SM_SentinelUndef - I'll make the change.

This revision was automatically updated to reflect the committed changes.