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
175

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.

211

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

245

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

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

346–347

missing comments

RKSimon added inline comments.Nov 4 2015, 8:04 AM
lib/Target/X86/InstPrinter/X86InstComments.cpp
346–347

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

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.