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

cranelift: Add inline stack probing for x64 #4747

Merged
merged 2 commits into from
Sep 1, 2022

Conversation

afonso360
Copy link
Contributor

👋 Hey,

This PR adds inline stack probing for the x64 backend. Opening as a draft since there are still some pending questions.

This is feature gated behind a enable_inline_probestack and also requires the user to enable enable_probestack.

Currently we have two implementations, If we only need to probe a few stack pages, we unroll the probes. If we need more we fall back to using a loop.

Closes: #2299
cc: @bjorn3 @cfallin

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift:meta Everything related to the meta-language. wasmtime:api Related to the API of the `wasmtime` crate itself labels Aug 21, 2022
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "cranelift:meta", "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@afonso360 afonso360 force-pushed the inline-stackprobing branch 2 times, most recently from 8b2a396 to e94273e Compare August 23, 2022 10:42
@afonso360 afonso360 marked this pull request as ready for review August 23, 2022 14:36
@jameysharp
Copy link
Contributor

This needs what I hope will be an easy rebase; cranelift/codegen/src/machinst/abi_impl.rs got merged into cranelift/codegen/src/machinst/abi.rs, so your changes just need to be applied to that file instead.

Otherwise, I think this looks like it does what you say it should do. @cfallin, now that it's implemented as a pseudo-instruction, what do you think?

@afonso360
Copy link
Contributor Author

Thanks for the reminder! I didn't realize this had a conflict.

There is an additional question that I'd like to ask for opinions. After this PR (as it is) we have two flags, enable_probestack and probestack_strategy. Should we merge those flags by adding a disabled arm to probestack_strategy?

@cfallin
Copy link
Member

cfallin commented Sep 1, 2022

Otherwise, I think this looks like it does what you say it should do. @cfallin, now that it's implemented as a pseudo-instruction, what do you think?

Looks great to me! I think this is the right approach. I didn't look closely at the emission code but the sequence in the comments seems reasonable, as does the unrolled-vs-looping optimization.

Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no opinion on whether to merge enable_probestack and probestack_strategy; I could imagine arguments both ways.

I have a couple questions/suggestions from doing another review pass but I think this would be fine to merge regardless.

cranelift/codegen/src/isa/x64/inst/emit.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/x64/inst/emit.rs Outdated Show resolved Hide resolved
@@ -1,5 +1,7 @@
test run
set enable_llvm_abi_extensions=true
; Disable stack probes since these tests don't require them
set enable_probestack=false
target x86_64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do just a few runtests need stack-probing disabled?

Copy link
Contributor Author

@afonso360 afonso360 Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests have stack slots above the threshold where we emit a stack probe (4k by default I think). The default strategy is to outline the stack probing to a external function call, but since we never provide an implementation for that anywhere the JIT can't link these tests, so they fail.

I manually disabled stack probing for these which reflects the old behaviour of force disabling it everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cranelift: Support stack probes without external function call
4 participants