This is an archive of the discontinued LLVM Phabricator instance.

[x86] Teach the X86 backend to DAG-combine SSE2 shuffles that are trivially redundant.
ClosedPublic

Authored by chandlerc on Jun 21 2014, 9:39 AM.

Details

Summary

This fixes several cases in the new vector shuffle lowering algorithm which would generate redundant shuffle instructions for the sake of simplicity.

I'm also deleting a testcase which was somewhat ridiculous. It was checking for a bug in 2007 about incorrectly transforming shuffles by looking for the string "-86" in the output of a pretty substantial function. This test case doesn't seem to have any value at this point.

Builds on top of: http://reviews.llvm.org/D4225

Diff Detail

Event Timeline

chandlerc updated this revision to Diff 10721.Jun 21 2014, 9:39 AM
chandlerc retitled this revision from to [x86] Teach the X86 backend to DAG-combine SSE2 shuffles that are trivially redundant..
chandlerc updated this object.
chandlerc edited the test plan for this revision. (Show Details)
chandlerc added reviewers: grosbach, filcab.
chandlerc set the repository for this revision to rL LLVM.
chandlerc added a subscriber: Unknown Object (MLST).
grosbach accepted this revision.Jun 23 2014, 11:19 AM
grosbach edited edge metadata.

This LGTM once the pre-req patch lands.

This revision is now accepted and ready to land.Jun 23 2014, 11:19 AM
filcab accepted this revision.Jun 23 2014, 2:55 PM
filcab edited edge metadata.

LGTM after the other patch, as well.
I just have a small comment on naming, but that basically relates to the whole file, unfortunately.

lib/Target/X86/X86ISelLowering.cpp
18826

Since we're already on the X86 backend, could we start calling these functions PerformX86ShuffleCombine? It's not like they're generic for more targets.

I know “Target” is what's being used everywhere else, but I'd prefer more specific naming.

Thanks, will submit once the other lands.

lib/Target/X86/X86ISelLowering.cpp
18826

I don't entirely disagree, but don't feel confident enough to cleanup the entire backend this way, and don't want to be inconsistent at this point, so I'd rather leave it.

chandlerc closed this revision.Jun 27 2014, 4:36 AM
chandlerc updated this revision to Diff 10923.

Closed by commit rL211889 (authored by @chandlerc).

lib/Target/X86/X86ISelLowering.cpp