[PATCH] D56633: [WebAssembly] Optimize BUILD_VECTOR lowering for size

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

All the inline comments disappeared because I switched to using the monorepo, which has different paths.

In D56633#1371855 <https://reviews.llvm.org/D56633#1371855>, @aheejin wrote:

>




> - How should undef elements be initialized? This patch looks it doesn't care about which number they are initialized with, whereas we initialize them with 0 in scalars. Is this OK?

Undef elements can be initialized with anything. We use zero here and elsewhere because there's no better default.



================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1169
+
+  // If v128.const is available, consider using it instead of a splat
+  if (Subtarget->hasUnimplementedSIMD128()) {
----------------
aheejin wrote:
> Can all these byte calculations change if we take LEB encoding into account?
Yes, LEB encoding can make any SIMD opcode arbitrarily long. I'm trying to use the smallest encoding in these calculations on the assumption that tools will always prefer the smallest encoding.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1173
+    const size_t LaneConstBytes = 3 + std::max(size_t(4), 16 / Lanes);
+    // splat or replace_lane with non-const argument on stack
+    const size_t LaneDynBytes = 2;
----------------
aheejin wrote:
> In which case you do splat for non-const arguments after using `v128.const`?
After using `v128.const`, everything else would be a `replace_lane`, not a splat. This value is also used for the initial splat when `v128.const` is not used, though.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1174
+    // splat or replace_lane with non-const argument on stack
+    const size_t LaneDynBytes = 2;
+    // v128.const and 128-bit value
----------------
aheejin wrote:
> For `replace_lane`, why is it 2? I guess 2 bytes for the opcode and a byte for immediate lane index, so 3?
Yes, good catch.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1184
+               // Constant replace_lanes
+               (NumConst - NumCommon) * LaneConstBytes +
+               // Dynamic replace_lanes
----------------
aheejin wrote:
> Why is this `LaneConstBytes` other than `LaneDynBytes`? Isn't `LaneDynBytes` the bytes needed to `replace_lane` instruction?
I assume (for a lack of better information) that all dynamic values are already on the stack, but all constants need to be materialized immediately before they are used. That means that for constant replace_lanes I count the replace_lane instruction and also the constant lane value.


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

https://reviews.llvm.org/D56633