[PATCH] D68527: [WebAssembly] v8x16.swizzle and rewrite BUILD_VECTOR lowering

Tue Oct 8 23:02:51 PDT 2019

aheejin accepted this revision.
aheejin added a comment.
This revision is now accepted and ready to land.

> We can make the decision based on whatever heuristic we want, but minimizing number of instructions seems like a good metric for now until we can run experiments to tune the selection algorithm.

Wouldn't minimizing the number of instruction be the same thing as minimizing the number of bytes, only more inaccurate?

> I don't know how swizzles and v128.const compare, but I do know that emulating swizzles requires a lot of instructions per lane but emulating a v128.const only requires a single replace_lane and constant per lane.

I don't understand this part well. If swizzles are a lot more complicated that `v128.const` in execution, doesn't that mean swizzles will likely to take longer to execute in wasm? Why the opposite?

The above are just some passing questions, but I'm not suggesting we restore the byte computation logic or another more complicated logic at this point, given that we don't have measurable any performance data at hand. Optimizing at this point seems too premature anyway. LGTM.

================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1384
+  SDValue SwizzleSrc;
+  SDValue SwizzleIndices;
+  size_t NumSwizzleLanes = 0;
----------------
aheejin wrote:
> Nit: Variable names for the same things in `GetSwizzleSrcs` are `SrcVec` and `IndexVec`. Making the variable names same in the two places might make reading easier.
In `GetSwizzleSrcs`, `IndexVec` is still `IndexVec`, while `SrcVec` was changed to `SwizzleSrc. Was that intentional?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68527