[PATCH] D51659: [WebAssembly] v8x16.shuffle

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
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();
----------------
aheejin wrote:
> 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.
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.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:986
+      Ops[OpIdx++] =
+          DAG.getConstant((uint64_t)Mask[I] * LaneBytes + J, DL, MVT::i32);
+    }
----------------
aheejin wrote:
> 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.
I believe this lowering happens after type legalization, so i32 is correct.


================
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>;
----------------
aheejin wrote:
> Doesn't this need a list of `SDTypeConstraint`s as the third argument?
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.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D51659