fitzgen requested cfallin for a review on PR #8876.
fitzgen requested wasmtime-compiler-reviewers for a review on PR #8876.
fitzgen opened PR #8876 from fitzgen:saferpoints-lowering
to bytecodealliance:main
:
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 packedUserStackMap
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.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
fitzgen updated PR #8876.
cfallin submitted PR review:
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.
cfallin submitted PR review:
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.
cfallin created PR review comment:
A little thing but rather than the explicit
+ 4
here, could we put thepush_user_stack_map()
call after the.put4()
below? That way the order of operations here matches the semantics of "stackmap at end of instruction" more directly.
cfallin created PR review comment:
(likewise here as above; coupling feels even a little more brittle since we're depending on the fact we emit two four-byte instructions below)
cfallin created PR review comment:
Should we assert that no more than one VCode inst lowered from a given CLIF inst is a safepoint-instruction? (In practice this should be the case as long as safepoints are calls only?) Duplicating the same CLIF-level stackmap for multiple VCode insts does seem correct-ish to me in the presence of a "the safepoint could actually happen at these N points" condition but is still somewhat surprising...
cfallin created PR review comment:
(likewise here as above)
cfallin created PR review comment:
Given the newtype idea above, could we have a
.to_insn_index(num_insts: usize)
on theBackwardInsnIndex
type?
cfallin created PR review comment:
Would it be possible to define a newtype over InsnIndex (
BackwardInsnIndex
) to make this a little more explicit? I don't think we need to weave it through the rest of lowering and translate out of it when doing the finalize/reverse, necessarily; just want to document in the types that this doesn't go through the same translation as the other index data.
fitzgen submitted PR review.
fitzgen created PR review comment:
Yeah I went back and forth on this and had that same reasoning about the stack maps being correct even if there are multiple safepoint mach insts, since nothing has moved those values in the middle of this CLIF instruction's lowering. But conservatively asserting there is only one is fine and something we can relax if we ever need to in the future (seems unlikely).
fitzgen updated PR #8876.
fitzgen submitted PR review.
fitzgen created PR review comment:
Done!
fitzgen submitted PR review.
fitzgen created PR review comment:
And done!
fitzgen has enabled auto merge for PR #8876.
fitzgen commented on PR #8876:
Thanks for the review, Chris!
fitzgen updated PR #8876.
fitzgen commented on PR #8876:
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 merged PR #8876.
Last updated: Nov 22 2024 at 16:03 UTC