[instantiation linking] Create and import WASMGlobalInstance #3914
Conversation
lum1n0us
force-pushed
the
feat/inst_linking_global
branch
from
92a6310 to
eb6a1ef
Compare
lum1n0us
force-pushed
the
feat/inst_linking_global
branch
2 times, most recently
from
bcb32d9 to
f4b1f30
Compare
lum1n0us
requested review from
loganek,
no1wudi,
wenyongh,
xujuntwt95329 and
yamt
as code owners
|
@lum1n0us there are some conflict files in this PR, could you help resolve them so that I can read the code better? Thanks. |
…d improve export retrieval logic Remove me when meeting rebase conflication
…l pointer retrieval
…dle global imports correctly
…creation for spec tests
…global definitions and removing unused code
…ltins for clarity
…able creation logic
…stance handling for shared memory
lum1n0us
force-pushed
the
feat/inst_linking_global
branch
from
a84546f to
36b8555
Compare
…antiate_with_builtin_linker
| aot_set_last_error("llvm build in bounds gep failed."); | ||
| return false; | ||
| } | ||
| global_ptr = get_global_value_addr(comp_ctx, func_ctx, global_idx); |
There was a problem hiding this comment.
The original code directly gets the global offset at the compilation time and loads global one time in execution. Per my understanding, get_global_value_addr will get the global address by several loads, it will reduce the performance? It seems that it is only needed for import global and only needed when multi-module is enabled?
There was a problem hiding this comment.
It seems that it is only needed for import global and only needed when multi-module is enabled?
In this case, multi-module means loading linking. By default, it is instantiation linking. Therefore, It always requires to prepare extra loading for an import global.
Yes. There are opportunities for optimization. Like:
- to a local global.
- to a built-in global.
I believe we should complete the functionality in this PR first. We can focus on optimization in a separate one.
There was a problem hiding this comment.
OK, since performance and code size are important for the developers, hope the new feature impacts them as less as we can.
| import_globals[i].is_linked = false; | ||
| else | ||
| #endif | ||
| #endif /* WASM_ENABLE_MULTI_MODULE != 0 */ |
There was a problem hiding this comment.
When multi-module isn't enabled and libc-builtin is enabled, the global isn't linked by default, is it expected? Or will the iwasm's main.c link the global when instantiating?
There was a problem hiding this comment.
YES. only apply wasm_native_lookup_libc_builtin_global() when executing loading linking. Otherwise, core/iwasm/libraries/libc-builtin/spec_test_builtin_wrapper.c will take over
| uint32 global_idx) | ||
| { | ||
| /* WASMModuleInstance->e */ | ||
| uint32 e_offset_val = offsetof(WASMModuleInstance, e); |
There was a problem hiding this comment.
The WASMModuleInstanceExtra structure is put at the end of WASMModuleInstance, we can use a fixed offset to get it and its fields, like offset_u32 = get_module_inst_extra_offset(comp_ctx) and then offset = offset_u32 + offsetof(AOTModuleInstanceExtra, globals) for AOT or offset = offset_u32 + offsetof(WASMModuleInstanceExtra, globals) for JIT. By this way, we can save one load operation, refer to create_shared_heap_info:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/compilation/aot_llvm.c#L1528-L1545
| LLVMValueRef e_ptr = | ||
| LLVMBuildBitCast(comp_ctx->builder, e_ptr_u8, OPQ_PTR_TYPE, "e_ptr"); | ||
| LLVMValueRef e = | ||
| LLVMBuildLoad2(comp_ctx->builder, OPQ_PTR_TYPE, e_ptr, "e"); |
There was a problem hiding this comment.
had better check return value of e_offset, e_ptr_u8, e_ptr, e
…clean up import instance handling
lum1n0us
merged commit b18cb3b
into
bytecodealliance:dev/instantiate_linking
2 participants
No description provided.