This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] v8x16.shuffle
ClosedPublic

Authored by tlively on Sep 4 2018, 2:13 PM.

Details

Summary

Since the shuffle mask is not exposed as an operand in the native ISel
DAG, create a new WebAssembly ISD node exposing the mask. The mask is
lowered as sixteen immediate byte indices no matter what type the
original vector shuffle was operating on.

This CL depends on D51656

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Sep 4 2018, 2:13 PM
tlively updated this revision to Diff 163929.Sep 4 2018, 3:06 PM
  • remove unrelated line change
aheejin added inline comments.Sep 6 2018, 3:49 PM
lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
227 ↗(On Diff #163929)

Is the size of these values i32? Isn't it LaneIdx32, which is a byte with values in the range 0–31? The same question for other places (in some of previous patches too)

tlively updated this revision to Diff 164343.Sep 6 2018, 6:09 PM
tlively marked an inline comment as done.
  • Switch shuffle operands to vec_i8imm_op

I'm checking. So according to the current shuffle spec, only half the number of elements from the two operands can be selected, right?

lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
973 ↗(On Diff #164343)

Maybe it's better to insert an if statement to check if the size of Mask array matches the vector's operand type so that it can safely be converted to v128 vector operations. For example, in case we have two 4xi32 vectors but the size of this Mask array is not equal to 4, the instruction selection should fail.

983 ↗(On Diff #164343)

When evaluating an end condition of a loop requires calling a function and the return value is an invariant throughout the loop, it is generally preferable to move the call to the init contidion, like
for (size_t I = 0, E = Mask.size(); I < E; ++I) {

Ref: https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop

986 ↗(On Diff #164343)

Shouldn't this be MVT::i8? We probably need to mask high bits and check if the value is the same after that I guess.

lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
316 ↗(On Diff #164343)

Doesn't this need a list of SDTypeConstraints as the third argument?

336 ↗(On Diff #164343)

Are these OK to be i32, when we changed the operand type for SHUFFLE_v16i8 to vec_i8imm_op?

tlively updated this revision to Diff 164478.Sep 7 2018, 11:37 AM
tlively marked an inline comment as done.
  • Fix loop condition and LaneBytes calculation
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
973 ↗(On Diff #164343)

I think it would be a failing elsewhere in LLVM if a mask array of the wrong size got this far, but I will rewrite this statement to be in terms of the operand types rather than based on the constant 16. That way if something goes wrong elsewhere in LLVM, the emitted dag node will have the wrong number of operands and isel will fail for lack of a matching pattern.

986 ↗(On Diff #164343)

I believe this lowering happens after type legalization, so i32 is correct.

lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
316 ↗(On Diff #164343)

Yes, but since this is not a public interface and because there are so many arguments, I didn't feel that it was worth it to specify the type constraints.

336 ↗(On Diff #164343)

Yes, since after type legalization there are no i8s.

aheejin accepted this revision.Sep 7 2018, 2:48 PM
This revision is now accepted and ready to land.Sep 7 2018, 2:48 PM
This revision was automatically updated to reflect the committed changes.