-
Notifications
You must be signed in to change notification settings - Fork 30
Fixes FP rounding issue and adds tests #166
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
base: main
Are you sure you want to change the base?
Conversation
1ee6aab
to
79fe90a
Compare
Nice catch 👍 LGTM. Did you check that the tests fail without the -soft-float,+sse,+sse2? |
Btw, I am not sure if you already have, but we should double check that we set all required cpu control registers for enabling sse,sse2. I know x86_64 assumes sse,sse2 are part base ISA, but still might need some setup. In addition, we should make sure these are reset between guest functions call to avoid any information leaks (if they aren't already). |
79fe90a
to
12c17c2
Compare
Yes. Also tested this against the sample in #86 (comment)
These seem to be enabled in KVM (I check the mxcsr register). I haven't verified on Windows/MSVH. Maybe the best thing do here it push hyperlight-dev/hyperlight#686 forward as well. Good call on clearing registers which will need to happen in HL, will work on a change for that. |
The issue was caused becuase cranelift compilation assume SSE2 even for the where as rust compiler doesn't include these instructions by default. This means when transitioning through wasmtime libcalls the parameters are lost since wasm is using SSE2 instructions and wasmtime isn't. The more advance SSE intructions require a seperate pr in HL core that is needed to enable them. Signed-off-by: James Sturtevant <[email protected]>
12c17c2
to
a3f7bd4
Compare
@@ -7,6 +7,8 @@ rustflags = [ | |||
"code-model=small", | |||
"-C", | |||
"link-args=-e entrypoint", | |||
"-C", | |||
"target-feature=-soft-float,+sse,+sse2" |
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.
turning off soft-float might cause issues with
hyperlight-wasm/src/wasm_runtime/src/marshal.rs
Lines 353 to 354 in 7775121
ReturnValue::Float(f) => Ok(Val::F32(f.to_bits())), | |
ReturnValue::Double(f) => Ok(Val::F64(f.to_bits())), |
fixes #86
The issue was caused because cranelift compilation assume SSE2 even for the where as rust compiler doesn't include these instructions by default. This means when transitioning through wasmtime libcalls the parameters are lost since wasm is using SSE2 instructions and wasmtime isn't. The more advance SSE instructions require a separate pr in HL core that is needed to enable them.