-
Notifications
You must be signed in to change notification settings - Fork 822
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
Add caching support for Singlepass backend. #1022
Conversation
bors try |
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.
Please activate this test:
wasmer/lib/runtime/src/cache.rs
Line 132 in dfc7163
#[cfg(all(test, not(feature = "singlepass")))] |
bors try- |
bors try |
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.
Question for you, how does this handle the code generated by Operator::F32Max which contains a pointer to the wasmer read-only data? Something like this:
static CANONICAL_NAN: u128 = 0x7FC0_0000;
// load float canonical nan
a.emit_mov(
Size::S64,
Location::Imm64((&CANONICAL_NAN as *const u128) as u64),
Location::GPR(tmpg1),
);
We can't assume the wasmer .rodata
will be in the same place in memory each time, due to ASLR.
/// Offsets to the beginnings of each function. (including trampoline, if any) | ||
function_pointers: Vec<usize>, | ||
|
||
/// Offsets to the beginnings of each function after trampoline. |
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.
Is this the absolute offset in the file of the function, or is this the number of bytes between the function and the trampoline? Also does this mean that the order in the on-disk file is <function1, trampoline1>, <function2, trampoline2>, ...?
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.
The absolute offset.
It is <trampoline1, function1>, <trampoline2, function2>, ... , though the trampolines only exist on AArch64.
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.
Thanks! Could you clarify this in the comments? Maybe at the top with:
/// On-disk cache format.
/// Offsets are relative to the start of the file.
/// Trampolines are only present on AArch64.
@nlewycky Didn't notice that we now have references as absolute addresses to outside data in the generated code - this itself is not expected and should be fixed. |
Co-Authored-By: nlewycky <nick@wasmer.io>
tryBuild succeeded |
OK. Unfortunately x86-64 doesn't have much of a way to put a constant into an xmm register except by going through memory. Maybe emit a jmp forward, write the constant bytes into the instruction stream, the load them as constant offset before %rip? |
This could also be updated: https://github.com/wasmerio/wasmer/blob/master/docs/feature_matrix.md |
ea510bb
to
c3f2481
Compare
Seems that we are already using a temporary GPR to keep the address of the constant. Since the values are at most 64 bits wide, why not do |
bors try |
tryBuild succeeded |
/// Offsets to the beginnings of each function. (including trampoline, if any) | ||
function_pointers: Vec<usize>, | ||
|
||
/// Offsets to the beginnings of each function after trampoline. |
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.
Thanks! Could you clarify this in the comments? Maybe at the top with:
/// On-disk cache format.
/// Offsets are relative to the start of the file.
/// Trampolines are only present on AArch64.
bors r+ |
Build succeeded |
Awesome! Looking forward to trying this out. |
This PR adds caching support for the Singlepass backend.