Page MenuHomePhabricator

[WebAssembly] Finalize wasm_simd128.h intrinsics
ClosedPublic

Authored by tlively on Apr 22 2021, 2:40 PM.

Details

Summary

Adds new intrinsics for instructions that are in the final SIMD spec but did not
previously have intrinsics. Also updates the names of existing intrinsics to
reflect the final names of the underlying instructions in the spec. Keeps the
old names as deprecated functions to ease the transition to the new names.

Diff Detail

Event Timeline

tlively created this revision.Apr 22 2021, 2:40 PM
tlively requested review of this revision.Apr 22 2021, 2:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2021, 2:40 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dschuff added inline comments.Apr 22 2021, 3:17 PM
clang/lib/Headers/wasm_simd128.h
171

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?

penzn accepted this revision.Apr 22 2021, 5:25 PM
penzn added a subscriber: penzn.

Looks good to me.

This revision is now accepted and ready to land.Apr 22 2021, 5:25 PM

General question about SIMD intrinsics: So we make dedicated wasm intrinsics only for the cases there are not general intrinsics people can use instead?

clang/lib/Headers/wasm_simd128.h
171

I was also curious about this too.

648

Do we not rename the builtin to v128 as well?

969

Sometimes builtin names seem slightly different from intrinsic names like this case: the intrinsic name is f32x4_ceil while the builtin name is ceil_f32x4. Is that intentional?

1389
1398

What does this macro do?

1547

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.

tlively added inline comments.Apr 23 2021, 10:24 AM
clang/lib/Headers/wasm_simd128.h
171

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

648

Yeah, I think it would be probably be more consistent to do so.

1389

Oops, thanks!

1398

This macro is defined when -Wdeprecated is enabled, so it can be used to detect when warnings about deprecated things should be emitted.

1547

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.

dschuff added inline comments.Apr 23 2021, 10:42 AM
clang/lib/Headers/wasm_simd128.h
171

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?

tlively updated this revision to Diff 340109.Apr 23 2021, 10:49 AM
  • "stand" => "standard"
  • Update builtins for v128.any_true

General question about SIMD intrinsics: So we make dedicated wasm intrinsics only for the cases there are not general intrinsics people can use instead?

No, every instruction in the proposal has an intrinsic so users don't need to worry about learning about the compiler's built-in vector support.

tlively added inline comments.Apr 23 2021, 1:04 PM
clang/lib/Headers/wasm_simd128.h
171

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.

tlively added inline comments.Apr 23 2021, 1:17 PM
clang/lib/Headers/wasm_simd128.h
969

Yes, the builtin names use _s and _u rather than communicating the sign in the lane interpretation and they always put the instruction prefix at the end. It's just a different arbitrary naming convention.

dschuff accepted this revision.Apr 23 2021, 1:24 PM
This revision was landed with ongoing or failed builds.Apr 23 2021, 1:37 PM
This revision was automatically updated to reflect the committed changes.
aheejin added inline comments.Apr 23 2021, 3:26 PM
clang/lib/Headers/wasm_simd128.h
171

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?

1547

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.

tlively added inline comments.Apr 23 2021, 3:53 PM
clang/lib/Headers/wasm_simd128.h
171

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.

1547

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.