Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add V128 SIMD value type #3412

Merged
merged 2 commits into from
May 14, 2024
Merged

Conversation

bnason-nf
Copy link
Contributor

Referencing a comment here:

#3363 (comment)

@wenyongh suggested that a 128 bit vector type should be added to the value type enum for SIMD support. This change adds an item to the relevant enums, and also adds code in various places to support that enum item. Please let me know if I missed anything.

Note that this requires changing the union inside the wasm_val_t structure to add a v128 member, which increases the overall size of the structure. As far as I can tell that doesn't cause any compatibility issues, even with AOT, but I'm not confident about that.

@@ -213,10 +213,11 @@ typedef struct WASMTag WASMTag;

#ifndef WASM_VALUE_DEFINED
#define WASM_VALUE_DEFINED

typedef union V128 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about that but it's a little confused to keep both union V128 and union wasm_v128_t, especially when they are sharing the same definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to change them to use the same name. Which do you prefer? I'm not completely sure about the naming rules, but using the name V128 in wasm_export.h didn't feel correct.

@wenyongh
Copy link
Contributor

Referencing a comment here:

#3363 (comment)

@wenyongh suggested that a 128 bit vector type should be added to the value type enum for SIMD support. This change adds an item to the relevant enums, and also adds code in various places to support that enum item. Please let me know if I missed anything.

Note that this requires changing the union inside the wasm_val_t structure to add a v128 member, which increases the overall size of the structure. As far as I can tell that doesn't cause any compatibility issues, even with AOT, but I'm not confident about that.

@bnason-nf is there any requirement from you to get v128 param/result type from func type? This really causes AOT ABI compatibility issue, the aot compiler assumes that the sizeof(wasm_val_t) is 16 when enabling the wamrc --invoke-c-api-import option:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/compilation/aot_emit_function.c#L360-L373

https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/doc/perf_tune.md#82-refine-callings-to-native-apis-registered-by-wasm-c-api-wasm_instance_new-from-aot-code

This PR means that the old AOT file generated by wamrc --invoke-c-api-import may be unable to run with new aot runtime since the offset isn't consistent.

@bnason-nf
Copy link
Contributor Author

@wenyongh yes that's what I was concerned about. However, this change is confusing to me then. Is the goal to add v128 to the value type enum, but not add v128 to the value type itself? Is there a use case for that?

@wenyongh
Copy link
Contributor

wenyongh commented May 13, 2024

@wenyongh yes that's what I was concerned about. However, this change is confusing to me then. Is the goal to add v128 to the value type enum, but not add v128 to the value type itself? Is there a use case for that?

@bnason-nf I think adding WASM_V128 to wasm_valkind_enum is good, and also we can enhance wasm_func_type_get_param_valkind/wasm_func_type_get_result_valkind. But for how to support v128 in wasm_val_t, there was discussion and it hasn't been supported in wasm-c-api and other runtimes yet, see:
WebAssembly/wasm-c-api#106
https://github.com/bytecodealliance/wasmtime/blob/main/crates/c-api/src/val.rs#L120

I think we had better keep wasm_val_t unchanged and seek better solution in the future.

For wamrc --invoke-c-api-import example, please refer to samples/wasm-c-api-import:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/samples/wasm-c-api-imports/wasm/CMakeLists.txt#L34

@bnason-nf
Copy link
Contributor Author

I see, thanks for the explanation. I've removed v128 from wasm_val_t.

Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bnason-nf
Copy link
Contributor Author

@wenyongh do you know why the checks for this PR are not running?

Copy link
Collaborator

@lum1n0us lum1n0us left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wenyongh
Copy link
Contributor

@wenyongh do you know why the checks for this PR are not running?

It is really strange, I have re-run the CIs manually.

@wenyongh wenyongh merged commit dbd8790 into bytecodealliance:main May 14, 2024
377 checks passed
@bnason-nf bnason-nf deleted the add_v128_value_type branch May 14, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants