Shared heap enhancement for AOT and update tests and samples #4226
Conversation
…rflow to accomodate shared heap chain
…chain iteration problem
…hared heap unit test
…and update shared heap unit tests and sample
|
The CI issue is due to the update of LLVM, which will be resolved after rebasing the dev branch |
TianlongLiang
requested review from
loganek,
lum1n0us,
no1wudi,
wenyongh,
xujuntwt95329 and
yamt
as code owners
| if (bytes == 0) { | ||
| bytes = 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
Theoretically, a large bytes can cause several troubles. Like underflow because shared_heap_end - bytes
| bh_static_assert(offsetof(WASMSharedHeap, size) == 32); | ||
| bh_static_assert(offsetof(WASMSharedHeap, start_off_mem64) == 40); | ||
| bh_static_assert(offsetof(WASMSharedHeap, start_off_mem32) == 48); | ||
|
|
There was a problem hiding this comment.
In my opinion, we should check all fields with DefPointer here to expose any potential AOT issues as early as possible. If agree, add next and heap_handle.
| @@ -77,6 +77,23 @@ extern "C" { | |||
| #undef DUMP_MODULE | |||
| #endif | |||
|
|
|||
| #if WASM_ENABLE_JIT != 0 && WASM_ENABLE_SHARED_HEAP != 0 | |||
There was a problem hiding this comment.
The macro definition can be moved to aot_llvm.c, and only one version should be kept.
| uint32 | ||
| get_module_inst_extra_offset(AOTCompContext *comp_ctx); | ||
|
|
||
| #define BUILD_FIELD_PTR(ptr, offset, field, name) \ |
| uint32 | ||
| get_module_inst_extra_offset(AOTCompContext *comp_ctx); | ||
|
|
||
| #define BUILD_FIELD_PTR(ptr, offset, field, name) \ |
There was a problem hiding this comment.
need a better name, like BUILD_LOAD_FIELD?
| LLVMBasicBlockRef check_succ, LLVMValueRef maddr_phi, | ||
| LLVMValueRef start_offset, LLVMValueRef max_addr, | ||
| LLVMValueRef mem_base_addr, LLVMValueRef bytes, uint32 bytes_u32, | ||
| bool is_memory64, bool is_target_64bit, bool bulk_memory, bool enable_segue) |
There was a problem hiding this comment.
Might need comments to explain what each variable stands for.
| LLVMBasicBlockRef check_succ, LLVMValueRef maddr_phi, | ||
| LLVMValueRef start_offset, LLVMValueRef max_addr, | ||
| LLVMValueRef mem_base_addr, LLVMValueRef bytes, uint32 bytes_u32, | ||
| bool is_memory64, bool is_target_64bit, bool bulk_memory, bool enable_segue) |
There was a problem hiding this comment.
maynot a good idea to cover two types of memory overflow check.
| if (is_memory64 && comp_ctx->enable_bound_check) { | ||
| /* 1.1 offset + addr can overflow when it's memory64 | ||
| * 2.1 Or when it's on 32-bit platform */ | ||
| if (is_memory64 || !is_target_64bit) { |
There was a problem hiding this comment.
miss the condition && comp_ctx->enable_bound_check)?
| BUILD_ICMP(LLVMIntULT, max_addr, offset, cmp, "cmp"); | ||
| BUILD_ICMP(LLVMIntNE, max_addr, is_target_64bit ? I64_ZERO : I32_ZERO, | ||
| cmp1, "cmp1"); | ||
| BUILD_OP(And, cmp, cmp1, cmp, "overflow"); |
There was a problem hiding this comment.
I thought L1448 can detect overflow, so why are the two below, L1449 and L1451 required?
| * otherwise it will be UINT32_MAX/UINT64_MAX */ | ||
| if (!(func_ctx->shared_heap_head_start_off = LLVMBuildSelect( | ||
| comp_ctx->builder, cmp, shared_heap_head_start_off_minus_one, | ||
| shared_heap_head_start_off, "head_start_off"))) { |
There was a problem hiding this comment.
If the comment is correct, we can use a const here without obtaining shared_heap_head_start_off.
For AOT:
For shared heap tests and samples: