As the WebAssembly SIMD proposal nears stabilization, there is desire
to use it with toolchains other than Emscripten. Moving the intrinsics
header to clang will make it available to WASI toolchains as well.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Very cool, thanks for putting this together!
clang/lib/Headers/wasm_simd128.h | ||
---|---|---|
11 | Do you know why other clang headers, such as lib/Headers/xmmintrin.h, don't use #pragma once? | |
31 | 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. | |
41 | In clang's xmmintrin.h, helper macros like these __DEFAULT_FN_ATTRS and __REQUIRE_CONSTANT are #undefed at the end of the file. | |
43 | This comment (and similar throughout the file) don't seem to be adding any new information. | |
88 | Similar to above, these can use the fixed-size type names like int64_t. | |
180 | c0, c1, etc. aren't reserved identifiers, so the convention in other clang headers is to prefix them with __. | |
294 | a, b, etc. also aren't reserved identifiers. |
clang/lib/Headers/wasm_simd128.h | ||
---|---|---|
11 | No, I don't know. According to https://github.com/emscripten-core/emscripten/pull/9299#discussion_r316893349 #pragma once is the convention in Emscripten's header files, but I don't know what the trade offs are. @sbc100 do you have a better answer for this? | |
43 | I agree. These have been around since the beginning of the file and we just kept adding them for consistency and never did anything else with them. I'll remove them now, but in the future (once the proposal is standardized?) we should put actual doc comments in their place, like xmmintrin.h has. |
clang/lib/Headers/wasm_simd128.h | ||
---|---|---|
11 | I think you should follow the local conventions. For whatever reason #pramga once never really seems to take off (emscripten is the only project I know of that uses it). |
- Prefix identifiers with __
- Use specific bit-widths where possible
- Do not change semantics under -funsigned-char
I believe all the feedback has now been addressed.
clang/lib/Headers/wasm_simd128.h | ||
---|---|---|
31 | Unfortunately not :( This change gives errors like error: cannot initialize a parameter of type '__attribute__((__vector_size__(16 * sizeof(char)))) char' (vector of 16 'char' values) with an rvalue of type '__i8x16' (vector of 16 'int8_t' values) return (v128_t)__builtin_wasm_abs_i8x16((__i8x16)a); Even changing char to signed char doesn't work. I would change the builtins to use the better types, but the builtin definition system does not allow those more interesting types. This is especially annoying because -funsigned-char does indeed change the behavior of the comparison intrinsics. I will have to do extra work in those functions to make it work. | |
41 | I can do that for __DEFAULT_FN_ATTRS but unfortunately not for __REQUIRE_CONSTANT because __REQUIRE_CONSTANT is present in the macro expansion of wasm_i8x16_const and friends. |
Cool, LGTM, with optional suggestion for signed char below:
clang/lib/Headers/wasm_simd128.h | ||
---|---|---|
31 | Would changing the clang definitions of the builtins to use signed char (Sc) explicitly fix this, like: -TARGET_BUILTIN(builtin_wasm_swizzle_v8x16, "V16cV16cV16c", "nc", "unimplemented-simd128") and so on for all the wasm simd builtins? |
clang/lib/Headers/wasm_simd128.h | ||
---|---|---|
31 | Neat, I hadn't remembered that those signed and unsigned prefixes existed. Yes, that would enable some simplifications, but I think they would be better done in a follow-on CL. |
Do you know why other clang headers, such as lib/Headers/xmmintrin.h, don't use #pragma once?