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: Take user stack maps through lowering and emission #8876

Merged
merged 4 commits into from
Jun 27, 2024

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Jun 26, 2024

Previously, user stack maps were inserted by the frontend and preserved in the mid-end. This commit takes them from the mid-end CLIF into the backend vcode, and then from that vcode into the finalized mach buffer during emission.

During lowering, we compile the UserStackMapEntrys into packed UserStackMaps. This is the appropriate moment in time to do that coalescing, packing, and compiling because the stack map entries are immutable from this point on.

Additionally, we include user stack maps in the Debug and disassembly implementations for vcode, just after their associated safepoint instructions. This allows us to see the stack maps we are generating when debugging, as well as write filetests that check we are generating the expected stack maps for the correct instructions.

Previously, user stack maps were inserted by the frontend and preserved in the
mid-end. This commit takes them from the mid-end CLIF into the backend vcode,
and then from that vcode into the finalized mach buffer during emission.

During lowering, we compile the `UserStackMapEntry`s into packed
`UserStackMap`s. This is the appropriate moment in time to do that coalescing,
packing, and compiling because the stack map entries are immutable from this
point on.

Additionally, we include user stack maps in the `Debug` and disassembly
implementations for vcode, just after their associated safepoint
instructions. This allows us to see the stack maps we are generating when
debugging, as well as write filetests that check we are generating the expected
stack maps for the correct instructions.

Co-Authored-By: Trevor Elliott <[email protected]>
@fitzgen fitzgen requested a review from a team as a code owner June 26, 2024 19:16
@fitzgen fitzgen requested review from cfallin and removed request for a team June 26, 2024 19:16
@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 labels Jun 26, 2024
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.

I really like this and look forward to seeing the original stackmap support deleted soon! (At that point I suppose we can rename "user stack maps" back to simply "stack maps" again?)

Some thoughts below but overall shape LGTM.

@fitzgen fitzgen enabled auto-merge June 27, 2024 18:22
@fitzgen
Copy link
Member Author

fitzgen commented Jun 27, 2024

Thanks for the review, Chris!

@fitzgen
Copy link
Member Author

fitzgen commented Jun 27, 2024

I really like this and look forward to seeing the original stackmap support deleted soon! (At that point I suppose we can rename "user stack maps" back to simply "stack maps" again?)

Yeah, next I will get Wasmtime using the new system, and then I will start removing the old system. Not 100% sure if I will spend the time to rename "user stack maps" to "stack maps", as the name kinda makes sense to me, given that these are really provided by the frontend "user" and the "user" is responsible for them (as opposed to being generated by Cranelift itself). But I don't feel strongly about it.

@fitzgen fitzgen added this pull request to the merge queue Jun 27, 2024
Merged via the queue into bytecodealliance:main with commit e20b424 Jun 27, 2024
66 checks passed
@fitzgen fitzgen deleted the saferpoints-lowering branch June 27, 2024 18:55
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 Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants