This is an archive of the discontinued LLVM Phabricator instance.

AVX-512 bit shuffle in 32-bit mode - fixed a bug
ClosedPublic

Authored by delena on Oct 11 2015, 11:41 PM.

Details

Summary

AVX-512 bit shuffle fails on 32 bit since we create a vector of 64-bit constants.
I split 8x64-bit const vector to 16x32 on 32-bit mode.

I added testing for 32-bit mode. I also removed the "bw" test from 8x64 vectors since it does not make any sense. (bw is for i8 and i16 types)

Diff Detail

Repository
rL LLVM

Event Timeline

delena updated this revision to Diff 37079.Oct 11 2015, 11:41 PM
delena retitled this revision from to AVX-512 bit shuffle in 32-bit mode - fixed a bug.
delena updated this object.
delena added reviewers: RKSimon, igorb.
delena set the repository for this revision to rL LLVM.
delena added a subscriber: llvm-commits.
RKSimon added inline comments.Oct 12 2015, 10:59 AM
../lib/Target/X86/X86ISelLowering.cpp
10746 ↗(On Diff #37079)

Is the constant build vector splitting worth putting into a helper function instead of bulking out this function? I haven't actually checked if there is similar code anywhere else (possibly vector variable shifts?)

../test/CodeGen/X86/vector-shuffle-512-v8.ll
2 ↗(On Diff #37079)

You are setting a 64-bit cpu and then overriding with a 32-bit triple? Most of the vector shuffle test files are just using triples.

29 ↗(On Diff #37079)

Should this be vmovdqa32?

delena updated this revision to Diff 37228.Oct 13 2015, 4:05 AM

Added a function getConstMaskVector() that creates a proper SDNode for mask.
I checked variable shift on 32-bit mode. I can't say that it is optimal, but correct and does not fail.

The vmovdqa64 instruction is selected instead of vmovdqa32 because the selection is going according to VT.
The final VT of the mask vector is v8i64.

RKSimon added inline comments.Oct 13 2015, 6:53 AM
../lib/Target/X86/X86ISelLowering.cpp
10756 ↗(On Diff #37228)

I meant use the helper to take VPermMask, split from i64 elements into 2xi32 if 32-bit, and create the BUILD_VECTOR - such a helper would then be useful for other situations where we need to legalize i64 build vectors on 32-bit targets. No need for getConstMaskVector to be shuffle mask specific.

I considered this approach, but when we deal with mask, we want to create UNDEF node for -1.
So the common helper may be useful, but not in this specific case.

Another possibility is to create a BUILD_VECTOR node from mask array and then rebuild it for 32-bit mode in a common helper.
Seems like a double work for 32.

  • Elena
delena updated this revision to Diff 37451.Oct 15 2015, 1:02 AM

Hi Simon,

I implemented more common variant for building a vector of constants. Please take a look.
thanks.

RKSimon accepted this revision.Oct 15 2015, 1:26 AM
RKSimon edited edge metadata.

LGTM - thanks for generalising the const helper function.

This revision is now accepted and ready to land.Oct 15 2015, 1:26 AM
This revision was automatically updated to reflect the committed changes.