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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Sorry for the delay! Some nits and questions.

- 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?



================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1137
+  auto IsConstant = [](const SDValue &V) {
+    return isa<ConstantSDNode>(V) || isa<ConstantFPSDNode>(V);
+  };
----------------
How about `V.getOpcode() == ISD::Constant || V.getOpcode() == ISD::ConstantFP` ?

`ConstantSDNode` and `ConstantFPSDNode`'s definitions seem to include `ISD::TargetConstant` and `ISD::TargetConstantFP`, whose definitions sound like we shouldn't do any optimizations on them.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1144
+  size_t NumUndef = 0, NumConst = 0, NumDynamic = 0;
+  for (size_t i = 0; i < Lanes; ++i) {
+    const SDValue &Lane = Op.getOperand(i);
----------------
Variable names should start with an uppercase letter. I know it looks especially weird for loop index variables ;( But anyway.. the same for all other for loops too.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1144
+  size_t NumUndef = 0, NumConst = 0, NumDynamic = 0;
+  for (size_t i = 0; i < Lanes; ++i) {
+    const SDValue &Lane = Op.getOperand(i);
----------------
aheejin wrote:
> Variable names should start with an uppercase letter. I know it looks especially weird for loop index variables ;( But anyway.. the same for all other for loops too.
How about `for (const SDValue &Op : Op.op_values())` ? For the other for loops too


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1147
+    if (Lane.isUndef()) {
+      NumUndef++;
+      continue;
----------------
This is not being used anywhere. Were you gonna use it for something? Otherwise we can delete this?


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1169
+
+  // If v128.const is available, consider using it instead of a splat
+  if (Subtarget->hasUnimplementedSIMD128()) {
----------------
Can all these byte calculations change if we take LEB encoding into account?


================
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;
----------------
In which case you do splat for non-const arguments after using `v128.const`?


================
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
----------------
For `replace_lane`, why is it 2? I guess 2 bytes for the opcode and a byte for immediate lane index, so 3?


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1184
+               // Constant replace_lanes
+               (NumConst - NumCommon) * LaneConstBytes +
+               // Dynamic replace_lanes
----------------
Why is this `LaneConstBytes` other than `LaneDynBytes`? Isn't `LaneDynBytes` the bytes needed to `replace_lane` instruction?


================
Comment at: test/CodeGen/WebAssembly/simd-build-vector.ll:116
+; CHECK-NEXT:  .functype       all_undef_i8x16 () -> (v128)
+; CHECK-NEXT:  return          $0
+define <16 x i8> @all_undef_i8x16() {
----------------
Looking at the code above, I wonder how we ended up with a `local.get` from an unassigned local here for all undef case. Actually it works beautifully, given that unassigned locals are zero-initialized by default and it is even less space than `splat 0`. Maybe this can be an optimization in which we try to replace all `v128.const 0, ..., 0` and `splat 0` with `local.get n` where n being an assigned local. Just a thought and not relevant to this CL.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56633