[PATCH] D88591: [WebAssembly] Emulate v128.const efficiently

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Nice idea! I believe this approach will be better in most cases; the only possible downside I can think of is something like this. In our test case there's this test:

  define <4 x i32> @splat_common_const_i32x4() {
    ret <4 x i32> <i32 undef, i32 3, i32 3, i32 1>
  } 

This is compiled to this currently:

  i32.const	$push0=, 3
  i32x4.splat	$push1=, $pop0
  i32.const	$push2=, 1
  i32x4.replace_lane	$push3=, $pop1, 3, $pop2

After this patch this becomes:

  i64.const	$push0=, 12884901888
  i64x2.splat	$push1=, $pop0
  i64.const	$push2=, 4294967299
  i64x2.replace_lane	$push3=, $pop1, 1, $pop2

The number of instructions is the same, but because we are now using `i64.const` rather than `i32.const`, can this increase the code size?



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1586
+    // Otherwise, if this is an integer vector, construct the constant from a
+    // pair of i64s.
+    std::array<uint64_t, 2> I64s({0, 0});
----------------
It'd be easier to read to have a little more comments here on what we are trying to do and why it is always better than the simpler splatting-and-replacing option which already existed. Something like [[ https://github.com/emscripten-core/emscripten/issues/12224#issuecomment-698048533 | this ]]. (Also it might be a good idea to link this issue in the CL/commit description for full reference).


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1601
+        auto *Const = cast<ConstantSDNode>(Lane.getNode());
+        uint64_t Val = byte_swap(Const->getLimitedValue(), little);
+        uint8_t *ValPtr = reinterpret_cast<uint8_t *>(&Val);
----------------
What does `getLimitedValue` do and why is it necessary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88591