This is an archive of the discontinued LLVM Phabricator instance.

[X86][SSE1] Add MOVLHPS/MOVHLPS lowering and memory folding support
ClosedPublic

Authored by RKSimon on Feb 7 2016, 3:40 AM.

Details

Summary

As discussed on PR26491, this patch adds support for lowering v4f32 shuffles to the MOVLHPS/MOVHLPS instructions. It also adds support for memory folding with their MOVLPS/MOVHPS load equivalents.

This first patch only really helps SSE1 targets as SSE2+ targets will widen the shuffle mask and use v2f64 equivalents (although they still combine to MOVLHPS/MOVHLPS for v2f64 splats). This will have to be addressed in a future patch, most likely when we add support for binary target shuffle combines.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 47121.Feb 7 2016, 3:40 AM
RKSimon retitled this revision from to [X86][SSE1] Add MOVLHPS/MOVHLPS lowering and memory folding support.
RKSimon updated this object.
RKSimon added reviewers: qcolombet, delena, andreadb, spatel.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: llvm-commits.
spatel edited edge metadata.Feb 7 2016, 9:08 AM

Shouldn't MOVHPS/MOVLPS/MOVLHPS/MOVHLPS be in "hasPartialRegUpdate()"? I wonder if the partial reg update is why we weren't trying to lower these before.

If that's correct, I think a FIXME comment is fine for now.

lib/Target/X86/X86InstrInfo.cpp
25–33

Extra indent here.

26

Does the AVX case doesn't need the alignment restriction? I don't think we'd actually generate this instruction in the first place if we have AVX, so it might be a moot point. But maybe worthy of a code comment.

RKSimon updated this revision to Diff 47147.Feb 7 2016, 12:13 PM
RKSimon edited edge metadata.

Updated based on Sanjay's comments.

spatel added inline comments.Feb 7 2016, 1:37 PM
lib/Target/X86/X86InstrInfo.cpp
5745

Should MOVHPS/MOVLPS/MOVLHPS/MOVHLPS be in this list?

RKSimon updated this revision to Diff 47187.Feb 8 2016, 6:31 AM

Thanks Sanjay - I missed that comment! The AMD 15h SOG highlights the MOVHP*/MOVLP* loads as having a merge dependency, I've added them to hasPartialRegUpdate().

spatel accepted this revision.Feb 8 2016, 7:06 AM
spatel edited edge metadata.

Please see another inline comment for hasPartialRegUpdate(). LGTM after that is addressed.

lib/Target/X86/X86InstrInfo.cpp
5763–5766

We're creating MOVLHPSrr / MOVHLPSrr with this patch, so I think those should be included too.

This revision is now accepted and ready to land.Feb 8 2016, 7:06 AM
This revision was automatically updated to reflect the committed changes.