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 a new resumable_trapnz instruction; #1869

Merged
merged 2 commits into from
Jun 15, 2020

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Jun 12, 2020

This is useful to have to allow resumable_trap to happen in loop
headers, for instance. This is the correct way to implement interrupt
checks in Spidermonkey, which are effectively resumable traps. Previous
implementation was using traps, which is wrong, since traps semantically
can't be resumed after.

@bnjbvr bnjbvr requested a review from cfallin June 12, 2020 11:02
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:meta Everything related to the meta-language. labels Jun 12, 2020
@github-actions
Copy link

Subscribe to Label Action

cc @bnjbvr

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:meta"

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

  • bnjbvr: cranelift

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

Learn more.

@bnjbvr
Copy link
Member Author

bnjbvr commented Jun 12, 2020

Just for context: the reason why I added a new instruction, rather than creating manually the blocks and use the resumable_trap instruction is that when lowering wasm to clif, we can't mutate the CFG in place (we can't create new blocks). Allow mutation of the CFG at this place would be nice in the future, and could allow new things (of the same kind that the IR template proposal would allow), but this is out of scope for what appears to be really a bug fix here.

@bnjbvr
Copy link
Member Author

bnjbvr commented Jun 12, 2020

Fuzz-target failure is being discussed in XAMPPRocky/remove_dir_all#19.

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@fitzgen
Copy link
Member

fitzgen commented Jun 12, 2020

https://github.com/bytecodealliance/wasmtime/blob/master/cranelift/codegen/src/regalloc/safepoint.rs#L54 should probably switch to something like

if pos.func.dfg[inst].is_resumable_trap() { ... }

that returns true for both resumable_trap and resumable_trapnz.

@bnjbvr bnjbvr force-pushed the resumable_trap_nz branch from 5a13fdd to f04455d Compare June 12, 2020 17:18
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks!

@bnjbvr
Copy link
Member Author

bnjbvr commented Jun 12, 2020

@fitzgen Thanks! Added something to that effect. (It didn't seem worth to make it a flag visible from the meta crate, but let me know if you think it's better (maybe for consistency? probably the other way around would be better imo))

@fitzgen
Copy link
Member

fitzgen commented Jun 12, 2020

(It didn't seem worth to make it a flag visible from the meta crate, but let me know if you think it's better (maybe for consistency? probably the other way around would be better imo))

Not 100% sure what you're getting at here, but the changes in this PR look great to me!

bnjbvr added 2 commits June 15, 2020 11:07
This is useful to have to allow resumable_trap to happen in loop
headers, for instance. This is the correct way to implement interrupt
checks in Spidermonkey, which are effectively resumable traps. Previous
implementation was using traps, which is wrong, since traps semantically
can't be resumed after.
@bnjbvr bnjbvr force-pushed the resumable_trap_nz branch from 9b800f4 to eac98ee Compare June 15, 2020 09:08
@bnjbvr bnjbvr merged commit 238ae3b into bytecodealliance:master Jun 15, 2020
@bnjbvr bnjbvr deleted the resumable_trap_nz branch June 15, 2020 10:04
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:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants