[PATCH] D89366: [WebAssembly] v128.load{8, 16, 32, 64}_lane instructions

[Heejin Ahn via Phabricator via cfe-commits]

aheejin accepted this revision.
aheejin added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/include/clang/Basic/BuiltinsWebAssembly.def:180
+TARGET_BUILTIN(__builtin_wasm_load32_lane, "V4iIii*", "nU", "simd128")
+TARGET_BUILTIN(__builtin_wasm_load64_lane, "V2LLiIiLLi*", "nU", "simd128")
+TARGET_BUILTIN(__builtin_wasm_store8_lane, "vV16ScIiSc*", "n", "simd128")
----------------
`U` in the third argument [[ https://github.com/llvm/llvm-project/blob/72732acade77d5ee55a818e2da77a2c5b7033ccb/clang/include/clang/Basic/Builtins.def#L75 | means ]] pure. Can we say loads are pure? (The same for existing `__builtin_wasm_load32_zero` and `__builtin_wasm_load64_zero`)


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16782
+    return Builder.CreateCall(Callee, {Vec, LaneIdx, Ptr});
+  }
   case WebAssembly::BI__builtin_wasm_shuffle_v8x16: {
----------------
Would it be better to follow the arguments order in the original proposal?  The order in [[ https://github.com/WebAssembly/simd/pull/350 | here ]] is mem, v, and lane. (I don't know why loads need `v` though) The same for intrinsic and instruction.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:246
+
+// TODO: Also support the oher load patterns for load_lane once the insructions
+// are merged to the proposal.
----------------



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:315
+defm : StoreLanePatNoOffset<v2i64, int_wasm_store64_lane, LaneIdx2>;
+
 //===----------------------------------------------------------------------===//
----------------



================
Comment at: llvm/test/CodeGen/WebAssembly/simd-load-lane-offset.ll:4
+
+; Test SIMD v128.load{8,16,32,64}_lane instructions. TODO: Use the offset field.
+
----------------
Maybe add a comment on which patterns are currently supported and unsupported among this file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89366