Wasmtime GitHub notifications bot (Jun 26 2024 at 19:16): Wasmtime GitHub notifications bot (Jun 26 2024 at 19:16):fitzgen requested cfallin for a review on PR #8876.
Wasmtime GitHub notifications bot (Jun 26 2024 at 19:16):fitzgen requested wasmtime-compiler-reviewers for a review on PR #8876.
Wasmtime GitHub notifications bot (Jun 26 2024 at 19:16):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
UserStackMapEntrys into packedUserStackMaps. 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
Debugand 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
-->
Wasmtime GitHub notifications bot (Jun 26 2024 at 19:25):fitzgen updated PR #8876.
Wasmtime GitHub notifications bot (Jun 27 2024 at 17:45):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.
Wasmtime GitHub notifications bot (Jun 27 2024 at 17:45):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.
Wasmtime GitHub notifications bot (Jun 27 2024 at 17:45):cfallin created PR review comment:
A little thing but rather than the explicit
+ 4here, 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.
Wasmtime GitHub notifications bot (Jun 27 2024 at 17:45):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)
Wasmtime GitHub notifications bot (Jun 27 2024 at 17:45):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...
Wasmtime GitHub notifications bot (Jun 27 2024 at 17:45):cfallin created PR review comment:
(likewise here as above)
Wasmtime GitHub notifications bot (Jun 27 2024 at 17:45):cfallin created PR review comment:
Given the newtype idea above, could we have a
.to_insn_index(num_insts: usize)on theBackwardInsnIndextype?
Wasmtime GitHub notifications bot (Jun 27 2024 at 17:45):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.
Wasmtime GitHub notifications bot (Jun 27 2024 at 17:53):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jun 27 2024 at 17:53):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).
Wasmtime GitHub notifications bot (Jun 27 2024 at 18:22):fitzgen updated PR #8876.
Wasmtime GitHub notifications bot (Jun 27 2024 at 18:22):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jun 27 2024 at 18:22):fitzgen created PR review comment:
Done!
Wasmtime GitHub notifications bot (Jun 27 2024 at 18:22):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jun 27 2024 at 18:22):fitzgen created PR review comment:
And done!
Wasmtime GitHub notifications bot (Jun 27 2024 at 18:22):fitzgen has enabled auto merge for PR #8876.
Wasmtime GitHub notifications bot (Jun 27 2024 at 18:23):fitzgen commented on PR #8876:
Thanks for the review, Chris!
Wasmtime GitHub notifications bot (Jun 27 2024 at 18:27):fitzgen updated PR #8876.
Wasmtime GitHub notifications bot (Jun 27 2024 at 18:29):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.
Wasmtime GitHub notifications bot (Jun 27 2024 at 18:55):fitzgen merged PR #8876.
Last updated: Dec 23 2024 at 12:05 UTC