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

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

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

225

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
36

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

379

missing comments

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

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
173

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.