[PATCH] D51659: [WebAssembly] v8x16.shuffle

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

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



================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:973
+  SDLoc DL(Op);
+  ArrayRef<int> Mask = cast<ShuffleVectorSDNode>(Op.getNode())->getMask();
+  size_t LaneBytes = 16 / Mask.size();
----------------
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.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:983
+  // Expand mask indices to byte indices and materialize them as operands
+  for (size_t I = 0; I < Mask.size(); ++I) {
+    for (size_t J = 0; J < LaneBytes; ++J) {
----------------
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


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:986
+      Ops[OpIdx++] =
+          DAG.getConstant((uint64_t)Mask[I] * LaneBytes + J, DL, MVT::i32);
+    }
----------------
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.


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:316
+// Shuffles after custom lowering
+def wasm_shuffle_t : SDTypeProfile<1, 18, []>;
+def wasm_shuffle : SDNode<"WebAssemblyISD::SHUFFLE", wasm_shuffle_t>;
----------------
Doesn't this need a list of `SDTypeConstraint`s as the third argument?


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:336
+            (i32 LaneIdx32:$mC), (i32 LaneIdx32:$mD),
+            (i32 LaneIdx32:$mE), (i32 LaneIdx32:$mF)))>;
+}
----------------
Are these OK to be `i32`, when we changed the operand type for `SHUFFLE_v16i8` to `vec_i8imm_op`?


Repository:
  rL LLVM

https://reviews.llvm.org/D51659