-
Notifications
You must be signed in to change notification settings - Fork 628
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
Fix compilation of shift opcodes on x86_64 and i386 architectures #2619
Conversation
e9ff4d3
to
2cbe758
Compare
tests/wamr-test-suites/wamr-compiler-test-scripts/run_wamr_compiler_tests.sh
Outdated
Show resolved
Hide resolved
tests/wamr-test-suites/wamr-compiler-test-scripts/run_wamr_compiler_tests.sh
Outdated
Show resolved
Hide resolved
call $i32_shr_s | ||
call $i32_shl | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about using assert_return
to check return values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, isn't that only available for WAST? Having said that, I can rename $assert_eq
to $assert_return
and move assertions to the _start
function if you think it's clearer.
This change fixes the case where the right parameter of shift operator is negative, specifically, when both parameters of shift opcode are constants. According to the WASM spec, negative value of the right parameter is allowed: > ishr_uN(i1, i2) > * Let k be i2 modulo N. > * Return the result of shifting i1 right by k bits, extended with 0 bits In general case, for x86 architecture wamrc compiler generates the following IR: ``` %shr_u = lshr i32 %"local0#", %"local1#" ``` Which (I guess, haven't verified it) is then translated to: ``` mov eax, %"local0#" mov ecx, %"local1#" shr eax, cl ``` so the assembly code already only uses the low 8 bits of the right parameter of shr_u. That's also why spec tests didn't fail, even though negative values were used. However, when both parameters of the shift operator are constant, LLVM performs optimization and calculates the value of the shift opcode at compile time here: https://github.com/llvm/llvm-project/blob/release/15.x/llvm/include/llvm/IR/ConstantFolder.h#L59 As part of the compile-time evaluation, LLVM checks if the right parameter is valid (i.e. smaller bit width of the left parameter): https://github.com/llvm/llvm-project/blob/release/15.x/llvm/lib/IR/ConstantFold.cpp#L1163 Because the right parameter is much bigger than bit width of the left parameter in case of negative value, compiler generates poisoned constant. Please note that the change does not affect the performance, as for constants the expression is still evaluated at compile time, i.e. for the code: ``` (func (export "_start") i32.const -1 i32.const -5 i32.shr_u i32.const 31 i32.ne if unreachable end ) ``` the IR output is: ``` define void @"aot_func#0"(ptr %exec_env) { func_begin: %aot_inst_addr = getelementptr inbounds ptr, ptr %exec_env, i32 2 %aot_inst = load ptr, ptr %aot_inst_addr, align 8 %cur_exception = getelementptr inbounds i8, ptr %aot_inst, i32 104 %func_ptrs_offset = getelementptr inbounds i8, ptr %aot_inst, i32 40 %func_ptrs_ptr = load ptr, ptr %func_ptrs_offset, align 8 br label %func_end func_end: ; preds = %func_begin ret void } ``` so we can see that shr_u was evaluated at compile time
2cbe758
to
6216d35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
When doing more investigations related to this: bytecodealliance#2619 I found that in some scenarios the constant might not be directly available to the LLVM IR builder, e.g.: ``` (func $const_ret (result i32) i32.const -5 ) (func $foo (i32.shr_u (i32.const -1) (call $const_ret)) (i32.const 31) ) ``` In that case, the right parameter to `i32.shr_u` is not constant, therefore the `SHIFT_COUNT_MASK` isn't applied. However, when the optimization is enabled (`--opt-level` is 2 or 3), the optimization passes resolve the call into constant, and that constant is poisoned, causing the compiler to resolve the whole function to an exception: ``` ; Function Attrs: mustprogress nofree norecurse nosync nounwind readnone willreturn define i32 @"aot_func#0"(ptr nocapture readonly %exec_env) local_unnamed_addr #0 { func_end: ret i32 -5 } define void @"aot_func#1"(ptr nocapture readonly %exec_env) local_unnamed_addr { got_exception: %aot_inst_addr = getelementptr inbounds ptr, ptr %exec_env, i64 2 %aot_inst1 = load i64, ptr %aot_inst_addr, align 8 %0 = inttoptr i64 %aot_inst1 to ptr tail call void @aot_set_exception_with_id(ptr %0, i32 0) ret void } ``` We'll enable the `SHIFT_COUNT_MASK` to avoid this unexpected behiavor. I'm not sure if that's something can be fixed in LLVM itself given that this is very WASM specific behavior.
When doing more investigations related to this PR: #2619 We found that in some scenarios the constant might not be directly available to the LLVM IR builder, e.g.: ``` (func $const_ret (result i32) i32.const -5 ) (func $foo (i32.shr_u (i32.const -1) (call $const_ret)) (i32.const 31) ) ``` In that case, the right parameter to `i32.shr_u` is not constant, therefore the `SHIFT_COUNT_MASK` isn't applied. However, when the optimization is enabled (`--opt-level` is 2 or 3), the optimization passes resolve the call into constant, and that constant is poisoned, causing the compiler to resolve the whole function to an exception.
…tecodealliance#2619) This change fixes the case where the right parameter of shift operator is negative, specifically, when both parameters of shift opcode are constants.
When doing more investigations related to this PR: bytecodealliance#2619 We found that in some scenarios the constant might not be directly available to the LLVM IR builder, e.g.: ``` (func $const_ret (result i32) i32.const -5 ) (func $foo (i32.shr_u (i32.const -1) (call $const_ret)) (i32.const 31) ) ``` In that case, the right parameter to `i32.shr_u` is not constant, therefore the `SHIFT_COUNT_MASK` isn't applied. However, when the optimization is enabled (`--opt-level` is 2 or 3), the optimization passes resolve the call into constant, and that constant is poisoned, causing the compiler to resolve the whole function to an exception.
This change fixes the case where the right parameter of shift operator is negative, specifically, when both parameters of shift opcode are constants.
According to the WASM spec, negative value of the right parameter is allowed:
In general case, for x86 architecture wamrc compiler generates the following IR:
Which (I guess, haven't verified it) is then translated to:
so the assembly code already only uses the low 8 bits of the right parameter of shr_u. That's also why spec tests didn't fail, even though negative values were used.
However, when both parameters of the shift operator are constant, LLVM performs optimization and calculates the value of the shift opcode at compile time here: https://github.com/llvm/llvm-project/blob/release/15.x/llvm/include/llvm/IR/ConstantFolder.h#L59 As part of the compile-time evaluation, LLVM checks if the right parameter is valid (i.e. smaller bit width of the left parameter): https://github.com/llvm/llvm-project/blob/release/15.x/llvm/lib/IR/ConstantFold.cpp#L1163 Because the right parameter is much bigger than bit width of the left parameter in case of negative value, compiler generates poisoned constant.
Please note that the change does not affect the performance, as for constants the expression is still evaluated at compile time, i.e. for the code:
the IR output is:
so we can see that shr_u was evaluated at compile time