[PATCH] D100018: [WebAssembly] Add shuffles as an option for lowering BUILD_VECTOR

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1640
+  // If the lane is extracted from another vector at a constant index, return
+  // that vector. The source vector must not have more lanes than the dest.
+  auto GetShuffleSrc = [&](const SDValue &Lane) {
----------------
> The source vector must not have more lanes than the dest.

Why? Shuffles can only include half of elements anyway, no?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1693
+      AddCount(ShuffleCounts, ShuffleSrc);
+    if (CanSwizzle) {
       auto SwizzleSrcs = GetSwizzleSrcs(I, Lane);
----------------
dschuff wrote:
> Unrelated to this CL but what exactly is the difference between a swizzle and a shuffle?
- Shuffle: https://github.com/WebAssembly/simd/blob/master/proposals/simd/SIMD.md#shuffling-using-immediate-indices
- Swizzle: https://github.com/WebAssembly/simd/blob/master/proposals/simd/SIMD.md#swizzling-using-variable-indices


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1711-1729
+  // Shuffles can draw from up to two vectors, so find the two most common
+  // sources.
+  SDValue ShuffleSrc1, ShuffleSrc2;
+  size_t NumShuffleLanes = 0;
+  if (ShuffleCounts.size()) {
+    std::tie(ShuffleSrc1, NumShuffleLanes) = GetMostCommon(ShuffleCounts);
+    ShuffleCounts.erase(std::remove_if(ShuffleCounts.begin(),
----------------
Does this handle when it is most beneficial to use two same vectors as both sources?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1735
   SDValue Result;
-  // Prefer swizzles over vector consts over splats
-  if (NumSwizzleLanes >= NumSplatLanes && NumSwizzleLanes >= NumConstantLanes) {
+  // Prefer swizzles over shuffles over vector consts over splats
+  if (NumSwizzleLanes >= NumShuffleLanes &&
----------------
Can we have some more comments on why we prefer this order?


================
Comment at: llvm/test/CodeGen/WebAssembly/simd-concat.ll:75
+; CHECK-NEXT:    local.get 1
+; CHECK-NEXT:    i8x16.shuffle 0, 1, 2, 3, 8, 9, 10, 11, 16, 17, 18, 19, 24, 25, 26, 27
+; CHECK-NEXT:    # fallthrough-return
----------------
I might be mistaken, but isn't this loss of data? v2i32 is a full 128bits vector and the result of `shufflevector` contain the whole two vectors but the result of `i8x16.shuffle` contains only half of it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100018/new/

https://reviews.llvm.org/D100018