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

Replace __yk_guardcheck with code patching #1428

Open
ltratt opened this issue Oct 21, 2024 · 4 comments
Open

Replace __yk_guardcheck with code patching #1428

ltratt opened this issue Oct 21, 2024 · 4 comments
Assignees

Comments

@ltratt
Copy link
Contributor

ltratt commented Oct 21, 2024

In deopt.rs we have:

/// When a guard fails, check if we have compiled a side-trace for this guard and if so, return
/// it's address. Otherwise return NULL, indicating that we need to deoptimise.
#[no_mangle]
pub(crate) extern "C" fn __yk_guardcheck(gidx: u64) -> *const libc::c_void { ... }

Lukas tasked me with looking at getting rid of this function by doing something with guards/guardinfos. On reflection, I've realised that would be pointless: we should be patching the code for guards when a side-trace is compiled, at which point the need for this function disappears entirely anyway!

This probably needs some sort of help from the (e.g. x64) code gen backend so that what is currently:

dynasm!(self.asm
    ; cmp Rb(reg.code()), inst.expect() as i8 // `as` intentional.
    ; jne =>fail_label
);

records the address of the jne so that later we can patch this to the equivalent of:

dynasm!(self.asm
    ; cmp Rb(reg.code()), inst.expect() as i8 // `as` intentional.
    ; jne =>sidetrace_whereever_it_is_in_memory
);

Probably we'll need to do some sort of nop tricks so that we always have enough space to patch the jump, but that's a relatively minor detail.

Amongst the neat properties of this change is that it should also absolve us of the need to check which side-trace we're currently executing: we just need to record the "top-level" trace we're in, and that's enough.

@ptersilie
Copy link
Contributor

Amongst the neat properties of this change is that it should also absolve us of the need to check which side-trace we're currently executing: we just need to record the "top-level" trace we're in, and that's enough.

I believe we still need the global guard vector change we've discussed. Otherwise we can't lookup the correct guard of a side-trace to store any sub-side-traces inside of it.

@ltratt
Copy link
Contributor Author

ltratt commented Oct 21, 2024

I believe we still need the global guard vector change we've discussed. Otherwise we can't lookup the correct guard of a side-trace to store any sub-side-traces inside of it.

Basically I think we can localise all the guards into the parent CompiledTrace rather than them being truly "global". And once that's done, we could e.g. search for an address X within the reachable set of traces from CompiledTrace.

@ptersilie
Copy link
Contributor

Yeah, whether it's global or per root trace makes no huge difference. But we need to do this whether we do patching or not, I think. So it might be a nice stepping stone to give us some speedup before we do patching.

@ptersilie
Copy link
Contributor

I've also now realised that we didn't discuss getting rid of __yk_guardcheck. It was __yk_reenter_jit that I think requires more immediate attention, and for which the storing all guards inside the root trace fix would also work.

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

No branches or pull requests

3 participants