This is an archive of the discontinued LLVM Phabricator instance.

AVX512 : VMOVSHDUP/VMOVSLDUP implementation.
ClosedPublic

Authored by igorb on Nov 3 2015, 11:40 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

igorb updated this revision to Diff 39169.Nov 3 2015, 11:40 PM
igorb retitled this revision from to AVX512 : VMOVSHDUP/VMOVSLDUP implementation..
igorb updated this object.
igorb added reviewers: delena, AsafBadouh.
igorb set the repository for this revision to rL LLVM.
igorb added a subscriber: llvm-commits.
delena added inline comments.Nov 4 2015, 1:42 AM
lib/Target/X86/InstPrinter/X86InstComments.cpp
155 ↗(On Diff #39169)

I propose to add some helper functions, like isZMMReg(), isYMMReg(), isXMMReg(),
it will simplify value type detection in some cases. And may be useful later.

191 ↗(On Diff #39169)

We can examine the number of operands in order see if the instruction has memory operand.

225 ↗(On Diff #39169)

remove empty line

igorb updated this revision to Diff 39177.Nov 4 2015, 2:47 AM
igorb marked 2 inline comments as done.

Thanks you for review!

delena added inline comments.Nov 4 2015, 4:12 AM
lib/Target/X86/InstPrinter/X86InstComments.cpp
37 ↗(On Diff #39177)

Operand index should be explicit.
The second parameter: MVT ScalarVT

348 ↗(On Diff #39177)

missing comments

RKSimon added inline comments.Nov 4 2015, 8:04 AM
lib/Target/X86/InstPrinter/X86InstComments.cpp
348 ↗(On Diff #39177)

Wouldn't it be better to match the 'fall through' pattern that we have for most rm / rr shuffle pairs instead of using operand count to re-determine if its rm? It would mean a splitting of the CASE_MOVDUP into 2 versions though.

igorb updated this revision to Diff 39325.Nov 4 2015, 11:37 PM
igorb marked 4 inline comments as done.

Thanks for your comments and suggestions.

RKSimon added inline comments.Nov 5 2015, 11:23 AM
lib/Target/X86/InstPrinter/X86InstComments.cpp
193 ↗(On Diff #39325)

We already have CASE_VSHUF_COMMON / CASE_VSHUF - is there any way that we can start standardizing these macros so that they can be reused as much as possible please?

igorb updated this revision to Diff 39648.Nov 8 2015, 12:40 AM
igorb marked an inline comment as done.

Thanks for your comments and suggestions.

LGTM - Elena should confirm for the actual AVX512 encodings however.

delena accepted this revision.Nov 13 2015, 11:42 PM
delena edited edge metadata.

LGTM

This revision is now accepted and ready to land.Nov 13 2015, 11:42 PM
This revision was automatically updated to reflect the committed changes.

It broke layering violation. Reproducible with BUILD_SHARED_LIBS=ON.

X86AsmPrinter might require X86Desc but X86Desc requires X86AsmPrinter.

igorb added a comment.Nov 15 2015, 4:22 AM

It broke layering violation. Reproducible with BUILD_SHARED_LIBS=ON.

X86AsmPrinter might require X86Desc but X86Desc requires X86AsmPrinter.

Thanks ,
Revert commit.