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

[instantiation linking] refactor wasm_table_inst_t #3901

Open
wants to merge 1 commit into
base: dev/instantiate_linking
Choose a base branch
from

Conversation

lum1n0us
Copy link
Collaborator

Use WASMTableInstance and (#define AOTModuleInstance WASMModuleInstance).

@lum1n0us
Copy link
Collaborator Author

@bnason-nf

It's not just for consistency. It's also beneficial for tasks such as creating a table from the host, importing an exported table from an instance, sharing table elements between the host and WebAssembly, between different WebAssembly modules, and for setting or getting a table from the host or another module. Additionally, it simplifies the management of ownership for wasm_table_inst_t.

@bnason-nf
Copy link
Contributor

Hi @lum1n0us,

I may be missing it, but I'm not seeing a way to access these (or equivalents) from the table after this refactor. Is there a way?

    wasm_valkind_t elem_kind;
    uint32_t cur_size;
    uint32_t max_size;

Thanks,
Benbuck

@lum1n0us
Copy link
Collaborator Author

I think there are two methods (possibly more). Please share your thoughts.

  • A new function wasm_runtime_get_export_index(const wasm_module_t, wasm_import_export_kind_t, const char*) retrieves the export_index using the name. An existing function wasm_runtime_get_export_type() returns wasm_export_t from the export_index. From wasm_export_t, since it's known to be a table, you can get wasm_table_type_t. Then, a series of existing functions wasm_table_type_get_XXX(const wasm_table_type_t) can complete the tasks. We could certainly add some helper functions to shorten this process.

  • Or A new function wasm_table_type_t wasm_runtime_get_table_type(xxx, wasm_table_inst_t) provides the wasm_table_type_t for a specified wasm_table_inst_t.

@bnason-nf
Copy link
Contributor

I guess the second way sounds nicer to me, but I don't have a strong opinion.

@lum1n0us lum1n0us force-pushed the fix/refactor_wasm_table_inst_t branch from d54c87a to dfb490e Compare November 15, 2024 00:25
@lum1n0us lum1n0us marked this pull request as draft November 15, 2024 02:07
@lum1n0us lum1n0us force-pushed the fix/refactor_wasm_table_inst_t branch from dfb490e to c0eee7a Compare November 15, 2024 07:08
@@ -704,6 +708,8 @@ tables_instantiate(AOTModuleInstance *module_inst, AOTModule *module,
tbl_inst->elem_ref_type.elem_ref_type =
module->tables[i].table_type.elem_ref_type;
#endif
tbl_inst->is_table64 = table->table_type.flags & TABLE64_FLAG;
tbl_inst->is_shared = table->table_type.flags & SHARED_TABLE_FLAG;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Still necessary. L751 will use that.

@lum1n0us lum1n0us force-pushed the fix/refactor_wasm_table_inst_t branch 2 times, most recently from 01c3c6c to 41f7526 Compare November 15, 2024 16:19
@lum1n0us
Copy link
Collaborator Author

Because there is no member such as WASMTableType in WASMTableInstance, there is always an issue with allocating and freeing resources for an API like wasm_table_type_t wasm_get_table_type(..., wasm_table_inst_t, ...).

Later, I understood that the actual issue isn't obtaining a wasm_table_type_t for any wasm_table_inst_t, but rather for an exported one. If the wasm_table_inst_t was created by the host, the type details are already known. So here is wasm_runtime_get_export_type_by_name().

@lum1n0us lum1n0us marked this pull request as ready for review November 15, 2024 16:40
@lum1n0us
Copy link
Collaborator Author

BTW, for the same purpose and idea, also need to refactor wasm_global_inst_t. If agreed, will do it another PR

@lum1n0us lum1n0us changed the base branch from main to dev/instantiate_linking November 19, 2024 03:19
@lum1n0us lum1n0us force-pushed the fix/refactor_wasm_table_inst_t branch from 41f7526 to 786ae05 Compare November 19, 2024 03:21
@lum1n0us lum1n0us changed the title refactor wasm_table_inst_t [instantiation linking] refactor wasm_table_inst_t Nov 20, 2024
@lum1n0us lum1n0us force-pushed the fix/refactor_wasm_table_inst_t branch from 786ae05 to 997c4aa Compare November 20, 2024 05:24
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.

2 participants