[PATCH] D76959: [WebAssembly] Import wasm_simd128.h from Emscripten

[Dan Gohman via Phabricator via cfe-commits]

sunfish added a comment.

Very cool, thanks for putting this together!



================
Comment at: clang/lib/Headers/wasm_simd128.h:10
+
+#pragma once
+
----------------
Do you know why other clang headers, such as `lib/Headers/xmmintrin.h`, don't use `#pragma once`?


================
Comment at: clang/lib/Headers/wasm_simd128.h:30
+typedef long long __i64x2 __attribute__((__vector_size__(16), __aligned__(16)));
+typedef unsigned long long __u64x2
+    __attribute__((__vector_size__(16), __aligned__(16)));
----------------
Since this file is already including <stdint.h>, instead of `char`, `unsigned char`, `short`, `unsigned short`, etc., can these use `int8_t`, `uint8_t`, `int16_t`, `uint16_t`, and so on, for clarity? Also, this would avoid any possible behavior change under `-funsigned-char`.


================
Comment at: clang/lib/Headers/wasm_simd128.h:40
+#define __REQUIRE_CONSTANT(e)                                                  \
+  _Static_assert(__builtin_constant_p(e), "Expected constant")
+
----------------
In clang's xmmintrin.h, helper macros like these `__DEFAULT_FN_ATTRS` and `__REQUIRE_CONSTANT` are `#undef`ed at the end of the file.


================
Comment at: clang/lib/Headers/wasm_simd128.h:42
+
+// v128 wasm_v128_load(void* mem)
+static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_v128_load(const void *__mem) {
----------------
This comment (and similar throughout the file) don't seem to be adding any new information.


================
Comment at: clang/lib/Headers/wasm_simd128.h:87
+  struct __wasm_v64x2_load_splat_struct {
+    long long __v;
+  } __attribute__((__packed__, __may_alias__));
----------------
Similar to above, these can use the fixed-size type names like `int64_t`.


================
Comment at: clang/lib/Headers/wasm_simd128.h:179
+    int8_t c7, int8_t c8, int8_t c9, int8_t c10, int8_t c11, int8_t c12,
+    int8_t c13, int8_t c14, int8_t c15) {
+  return (v128_t)(__i8x16){c0, c1, c2,  c3,  c4,  c5,  c6,  c7,
----------------
`c0`, `c1`, etc. aren't reserved identifiers, so the convention in other clang headers is to prefix them with `__`.


================
Comment at: clang/lib/Headers/wasm_simd128.h:293
+// v128_t wasm_i8x16_splat(int8_t a)
+static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_i8x16_splat(int8_t a) {
+  return (v128_t)(__i8x16){a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a};
----------------
`a`, `b`, etc. also aren't reserved identifiers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76959