This is an archive of the discontinued LLVM Phabricator instance.

[X86][SSE] Shuffle mask decode support for zero extend, scalar float/double moves and integer load instructions
ClosedPublic

Authored by RKSimon on Jan 28 2015, 9:01 AM.

Details

Summary

This patch adds shuffle mask decodes for integer zero extends (pmovzx** and movq xmm,xmm) and scalar float/double loads/moves (movss/movsd).

Also adds shuffle mask decodes for integer loads (movd/movq).

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 18896.Jan 28 2015, 9:01 AM
RKSimon retitled this revision from to [X86][SSE] Shuffle mask decode support for zero extend, scalar float/double moves and integer load instructions.
RKSimon updated this object.
RKSimon edited the test plan for this revision. (Show Details)
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: Unknown Object (MLST).
qcolombet added inline comments.Jan 28 2015, 5:33 PM
lib/Target/X86/InstPrinter/X86InstComments.cpp
24 ↗(On Diff #18896)

Instead of returning SrcVT, I think, based on the uses of that function, that it would be better to return what you called the scale.
If you want to stick with SrcVT, see my next comment.

34 ↗(On Diff #18896)

The SrcVT is confusing to me.
The used input type is v8i8, not v16i8.
But indeed, the “register” type is v16i8.
If you want to use SrcVT, please clarify what is the expected output of this function.

Note: I like the use of SrcVT, instead of scale, but I found the types set here confusing.

So my suggestion is do either of:

  1. Return Scale.
  2. Add a comment to explain the return SrcVT is the content of the whole register, not just that is used.
  3. Set SrcVT to what is actually used.
lib/Target/X86/Utils/X86ShuffleDecode.cpp
428 ↗(On Diff #18896)

Format of the comments. Capital letter and period.

RKSimon updated this revision to Diff 19028.Jan 30 2015, 3:05 AM

Thanks Quentin, I've updated the comments in the patch to explain that the SrcVT type represents the whole register, not just the lower elements that will be zero extended in the DstVT.

qcolombet accepted this revision.Jan 30 2015, 9:48 AM
qcolombet edited edge metadata.

Hi Simon,

LGTM.

Thanks,
-Quentin

This revision is now accepted and ready to land.Jan 30 2015, 9:48 AM
This revision was automatically updated to reflect the committed changes.