Page MenuHomePhabricator

[x86] Combine x86mmx/i64 to v2i64 conversion to use scalar_to_vector
ClosedPublic

Authored by bruno on Jan 22 2015, 5:10 AM.

Details

Summary

Handle the poor codegen for i64/x86xmm->v2i64 (%mm -> %xmm) moves. Instead of using stack store/load pair to do the job, use scalar_to_vector directly, which in the MMX case can use movq2dq. This was the current behavior prior to improvements for vector legalization of extloads in r213897. This patch fixes the regression and as a side-effect also remove some unnecessary shuffles.

In the new attached testcase, we go from:

pshufw $-18, (%rdi), %mm0
movq %mm0, -8(%rsp)
movq -8(%rsp), %xmm0
pshufd $-44, %xmm0, %xmm0
movd %xmm0, %eax
...

To:

pshufw $-18, (%rdi), %mm0
movq2dq %mm0, %xmm0
movd %xmm0, %eax
...

Diff Detail

Repository
rL LLVM

Event Timeline

bruno updated this revision to Diff 18606.Jan 22 2015, 5:10 AM
bruno retitled this revision from to [x86] Combine x86mmx/i64 to v2i64 conversion to use scalar_to_vector.
bruno updated this object.
bruno edited the test plan for this revision. (Show Details)
bruno added reviewers: chandlerc, spatel, nadav.
bruno set the repository for this revision to rL LLVM.
bruno added a subscriber: Unknown Object (MLST).
spatel accepted this revision.Jan 22 2015, 10:41 AM
spatel edited edge metadata.

See inline comments for nitpicks. Otherwise, LGTM.

lib/Target/X86/X86ISelLowering.cpp
24738

"Conversion"

24741

"redundant"

24746

Period at end of sentence.

24754

Period at end of sentence.

test/CodeGen/X86/2012-01-18-vbitcast.ll
5

I was told that we should always put a space between the ';' and 'CHECK'. And yes, I realize that a very large fraction of the existing regression tests do not do this. :)

This revision is now accepted and ready to land.Jan 22 2015, 10:41 AM
bruno closed this revision.Jan 23 2015, 2:46 PM

Updated patch with Sanjay comments and committed in r226953

chandlerc edited edge metadata.Jan 23 2015, 3:13 PM

Generally nice. Some further comments to spatel's inline. I'm happy with the change once spatel is, and with whatever approach you take for cleaning up the test cases.

lib/Target/X86/X86ISelLowering.cpp
24754

Also, I would use "store" rather than "save".

test/CodeGen/X86/2012-01-18-vbitcast.ll
5

My preference is to convert tests to use the utils/update_llc_test_checks.py script when appropriate (IE, when they are testing a microscopic and very specific sequence of instructions). Where we can, I'd also appreciate merging test cases out of <date>-foo.ll files and into reasonably semantically named files.

I'm not too fussed about what order this stuff happens in, but I usually find it easier to review patches when the test cases are moved or regularized with the script first as it tends to make the diff cleaner.

Anyways, not super important to this *specific* patch, but hopefully useful going forward.

test/CodeGen/X86/mmx-movq2dq.ll
1

How about a generic 'vector-shuffle-mmx.ll' test file that we can consolidate MMX-specific shuffle and mov test cases into?