Add wasi_ephemeral_nn module support
#3241
Conversation
|
@tonibofarull could you help review this PR? Thanks a lot. |
core/iwasm/common/wasm_native.c
Outdated
| if (!wasm_native_register_natives("wasi_nn", native_symbols, | ||
| n_native_symbols)) | ||
| goto fail; | ||
| #if WASM_ENABLE_WASI_EPHEMERAL_NN != 0 | ||
| n_native_symbols = get_wasi_ephemeral_nn_export_apis(&native_symbols); | ||
| if (!wasm_native_register_natives("wasi_ephemeral_nn", native_symbols, | ||
| n_native_symbols)) | ||
| goto fail; | ||
| #endif /* WASM_ENABLE_WASI_EPHEMERAL_NN != 0 */ |
There was a problem hiding this comment.
I strongly believe that we shouldn't replicate the code but instead define here wasi_nn or wasi_ephemeral_nn as part of the configuration.
There was a problem hiding this comment.
Before deprecating wasi_nn I'd give the option to select one or the other.
There was a problem hiding this comment.
I strongly believe that we shouldn't replicate the code but instead define here
wasi_nnorwasi_ephemeral_nnas part of the configuration.
It seems there are slight difference in this file between original wasi_nn_xx() and newly added wasi_ephemeral_nn_xx(). I am wondering can merge both?
There was a problem hiding this comment.
I'd consider on supporting only 1 at a time and update the implementation of wasi_nn_xx and wasi_ephemeral_nn, if there are any.
AuYang261
force-pushed
the
dev/wasi_ephemeral_nn
branch
from
7cbf4be
to
e9e7906
Compare
|
@AuYang261 I have just merged PR #3246 to disable running CodeQL CI on each pull request, or it will run for a long time and fail eventually. Could you please rebase your branch with WAMR main? |
No problem. |
AuYang261
force-pushed
the
dev/wasi_ephemeral_nn
branch
from
e9e7906
to
ddff5c8
Compare
|
@AuYang261, @tonibofarull the code seems good now, I merged the PR. Please submit a new PR to enhance it if needed. |
Add `wasi_ephemeral_nn` module support with optional cmake variable, which was mentioned in bytecodealliance#3229.
Add
wasi_ephemeral_nnmodule support with optional cmake variable, which mentioned in #3229.