This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Import wasm_simd128.h from Emscripten
ClosedPublic

Authored by tlively on Mar 27 2020, 4:37 PM.

Details

Summary

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.

Diff Detail

Event Timeline

tlively created this revision.Mar 27 2020, 4:37 PM
tlively updated this revision to Diff 253249.Mar 27 2020, 4:40 PM
  • Update license to match xmmintrin.h format

Very cool, thanks for putting this together!

clang/lib/Headers/wasm_simd128.h
10

Do you know why other clang headers, such as lib/Headers/xmmintrin.h, don't use #pragma once?

30

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.

40

In clang's xmmintrin.h, helper macros like these __DEFAULT_FN_ATTRS and __REQUIRE_CONSTANT are #undefed at the end of the file.

42

This comment (and similar throughout the file) don't seem to be adding any new information.

87

Similar to above, these can use the fixed-size type names like int64_t.

179

c0, c1, etc. aren't reserved identifiers, so the convention in other clang headers is to prefix them with __.

293

a, b, etc. also aren't reserved identifiers.

tlively marked 2 inline comments as done.Mar 27 2020, 6:04 PM
tlively added inline comments.
clang/lib/Headers/wasm_simd128.h
10

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?

42

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.

sbc100 added inline comments.Mar 27 2020, 7:09 PM
clang/lib/Headers/wasm_simd128.h
10

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).

tlively updated this revision to Diff 253703.Mar 30 2020, 2:37 PM
tlively marked 3 inline comments as done.
  • Prefix identifiers with __
  • Use specific bit-widths where possible
  • Do not change semantics under -funsigned-char
tlively updated this revision to Diff 253713.Mar 30 2020, 3:07 PM
  • Use header guard macro instead of pragma

I believe all the feedback has now been addressed.

clang/lib/Headers/wasm_simd128.h
30

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.

40

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.

sunfish accepted this revision.Mar 30 2020, 4:49 PM

Cool, LGTM, with optional suggestion for signed char below:

clang/lib/Headers/wasm_simd128.h
30

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")
+TARGET_BUILTIN(
builtin_wasm_swizzle_v8x16, "V16ScV16ScV16Sc", "nc", "unimplemented-simd128")

and so on for all the wasm simd builtins?

This revision is now accepted and ready to land.Mar 30 2020, 4:49 PM
tlively marked an inline comment as done.Mar 30 2020, 5:03 PM
tlively added inline comments.
clang/lib/Headers/wasm_simd128.h
30

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.

This revision was automatically updated to reflect the committed changes.