This is an archive of the discontinued LLVM Phabricator instance.

[X86][SSE] Keep 32-bit target i64 vector shifts on SSE unit.
ClosedPublic

Authored by RKSimon on Jul 18 2015, 7:27 AM.

Details

Summary

This patch improves the 32-bit target i64 constant matching to detect the shuffle vector splats that are introduced by i64 vector shift vectorization (D8416).

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 30079.Jul 18 2015, 7:27 AM
RKSimon retitled this revision from to [X86][SSE] Keep 32-bit target i64 vector shifts on SSE unit..
RKSimon updated this object.
RKSimon added reviewers: qcolombet, delena, spatel, andreadb.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: llvm-commits.
qcolombet added inline comments.Jul 28 2015, 2:11 PM
lib/Target/X86/X86ISelLowering.cpp
22

We should check that the SplatIndex is covered by the first operand of Amt, otherwise the following code is wrong.

RKSimon updated this revision to Diff 30887.Jul 29 2015, 3:28 AM

Thanks Quentin - updated to get the correct operand based on splat index.

qcolombet accepted this revision.Jul 29 2015, 9:53 AM
qcolombet edited edge metadata.

Hi Simon,

Thanks for the updated version. I have mixed feeling about it though. Indeed, I think the code is correct, but given how the lowering works, I believe we cannot reach it.
In other words, currently, I do not think we can produce a test case that covers this code. If you can add a test case to cover it, then awesome :).

How about not doing the transformation when the splat index is not covered by the first operand, so that if it happens we would have a test case to add with the code support it?

Other than that LGTM, no need to wait for a new review.

Thanks,
-Quentin

This revision is now accepted and ready to land.Jul 29 2015, 9:53 AM

Thanks for the updated version. I have mixed feeling about it though. Indeed, I think the code is correct, but given how the lowering works, I believe we cannot reach it.
In other words, currently, I do not think we can produce a test case that covers this code. If you can add a test case to cover it, then awesome :).

How about not doing the transformation when the splat index is not covered by the first operand, so that if it happens we would have a test case to add with the code support it?

Yes the canonicalization in DAG.getVectorShuffle /should/ stop it happening. Would adding an assert to check the splat comes from the first operand be too harsh?

I’m fine with an assert.

Thanks,
-Quentin

This revision was automatically updated to reflect the committed changes.

I’m fine with an assert.

All done. Thanks Quentin.