[PATCH] D101112: [WebAssembly] Finalize wasm_simd128.h intrinsics

[Thomas Lively via Phabricator via cfe-commits]

tlively added inline comments.


================
Comment at: clang/lib/Headers/wasm_simd128.h:171
+
+#define wasm_v128_load8_lane(__ptr, __vec, __i)                                \
+  ((v128_t)__builtin_wasm_load8_lane((signed char *)(__ptr), (__i8x16)(__vec), \
----------------
aheejin wrote:
> tlively wrote:
> > dschuff wrote:
> > > tlively wrote:
> > > > aheejin wrote:
> > > > > dschuff wrote:
> > > > > > out of curiosity, why are these macros, while all the rest (including ones that don't need declarations such as `wasm_i64x2_eq`) seem to be inline functions?
> > > > > I was also curious about this too.
> > > > The `i` parameter needs to be an integer constant, and I never figured out a way to enforce that for a function parameter. (But using a macro works because the codegen for the builtin functions can error out on non-constant arguments.)
> > > Ah, that makes sense. It does make me wonder, do we have any documentation about those constraints? I guess this file itself is more-or-less what we have, right? If the constraint is violated, is the error from the compiler intelligible?
> > Yes, the error should be somewhat intelligible, as far as error messages go. So far this is all we have, but I would like to get proper documentation at some point.
> > The `i` parameter needs to be an integer constant, and I never figured out a way to enforce that for a function parameter. (But using a macro works because the codegen for the builtin functions can error out on non-constant arguments.)
> 
> Why is a function and a macro on this? Doesn't a function also call our builtin functions, which errors out on non-const arguments?
With a function that calls the builtin function, the builtin function is passed one of the parameters to the outer function and parameters are never integer constants (this is all resolved before inlining, I guess). When an integer constant is an argument to a macro, it gets copy-pasted to be the argument to the builtin function, so everything works out.


================
Comment at: clang/lib/Headers/wasm_simd128.h:1547
+static __inline__ v128_t __DEPRECATED_FN_ATTRS("wasm_u16x8_extend_low_u8x16")
+wasm_i16x8_widen_low_u8x16(v128_t __a) {
+  return wasm_u16x8_extend_low_u8x16(__a);
----------------
aheejin wrote:
> tlively wrote:
> > aheejin wrote:
> > > The deprecated function name and the new intrinsic say `u`. Should this be `u` too? I haven't checked every single entry, but there seem to be multiple instances like this.
> > No, the old name had an `i` here (see the change on 1059). The convention is to use `u` and `i` to communicate unsignedness and signedness. Previously this name just had a `u` on the `u8x16` at the end, which was sufficient to communicate whether this was the `_u` or `_s` variant of the instruction. For the new names, I'm trying to communicate signedness of both the parameters and results, so the new name uses a `u` in both locations. In contrast, e.g. `wasm_u16x8_narrow_i32x4` remains unchanged because the inputs to narrowing operations are treated as signed even if the output is unsigned.
> Sorry maybe I'm mistaken, but what I meant was, the function name here is 
> `wasm_i16x8_widen_low_u8x16`
> where `i` on 16x8 and `u` on 8x16
> 
> But the call in the line below is
> `return wasm_u16x8_extend_low_u8x16(__a)`
> where `u` is both on 16x8 and 8x16.
Yes, that is an intentional part of the name change, in addition to "widen" being replaced with "extend". The deprecated function with the `i` on the `i16x8` is calling the new function with the `u` in both locations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101112