Skip to content
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

Merged
merged 9 commits into from
Dec 2, 2019
Merged

Conversation

losfair
Copy link
Contributor

@losfair losfair commented Nov 27, 2019

This PR adds caching support for the Singlepass backend.

  • Implementation
  • AArch64 test

@losfair
Copy link
Contributor Author

losfair commented Nov 27, 2019

bors try

@losfair losfair marked this pull request as ready for review November 27, 2019 18:55
bors bot added a commit that referenced this pull request Nov 27, 2019
Copy link
Contributor

@bjfish bjfish left a 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:

#[cfg(all(test, not(feature = "singlepass")))]

@losfair
Copy link
Contributor Author

losfair commented Nov 27, 2019

bors try-

@losfair
Copy link
Contributor Author

losfair commented Nov 27, 2019

bors try

bors bot added a commit that referenced this pull request Nov 27, 2019
Copy link
Contributor

@nlewycky nlewycky left a 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.

lib/singlepass-backend/src/codegen_x64.rs Outdated Show resolved Hide resolved
/// Offsets to the beginnings of each function. (including trampoline, if any)
function_pointers: Vec<usize>,

/// Offsets to the beginnings of each function after trampoline.
Copy link
Contributor

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>, ...?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@losfair
Copy link
Contributor Author

losfair commented Nov 27, 2019

@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>
@bors
Copy link
Contributor

bors bot commented Nov 27, 2019

try

Build succeeded

@nlewycky
Copy link
Contributor

@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.

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?

@bjfish
Copy link
Contributor

bjfish commented Nov 27, 2019

This could also be updated: https://github.com/wasmerio/wasmer/blob/master/docs/feature_matrix.md

@losfair
Copy link
Contributor Author

losfair commented Nov 28, 2019

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 mov imm{32,64}, gpr and then mov gpr, xmm directly? (updated code with this approach, waiting on tests)

@losfair
Copy link
Contributor Author

losfair commented Nov 28, 2019

bors try

bors bot added a commit that referenced this pull request Nov 28, 2019
@bors
Copy link
Contributor

bors bot commented Nov 28, 2019

try

Build succeeded

lib/singlepass-backend/src/codegen_x64.rs Outdated Show resolved Hide resolved
/// Offsets to the beginnings of each function. (including trampoline, if any)
function_pointers: Vec<usize>,

/// Offsets to the beginnings of each function after trampoline.
Copy link
Contributor

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.

@losfair
Copy link
Contributor Author

losfair commented Dec 2, 2019

bors r+

bors bot added a commit that referenced this pull request Dec 2, 2019
1022: Add caching support for Singlepass backend. r=losfair a=losfair

This PR adds caching support for the Singlepass backend.

- [x] Implementation
- [x] AArch64 test

Co-authored-by: losfair <zhy20000919@hotmail.com>
Co-authored-by: Heyang Zhou <zhy20000919@hotmail.com>
@bors
Copy link
Contributor

bors bot commented Dec 2, 2019

Build succeeded

@bors bors bot merged commit 2eb11f5 into master Dec 2, 2019
@bors bors bot deleted the feature/singlepass-cache branch December 2, 2019 18:43
@ethanfrey
Copy link
Contributor

Awesome! Looking forward to trying this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants