Stream: git-wasmtime

Topic: wasmtime / PR #8876 Cranelift: Take user stack maps throu...


view this post on Zulip Wasmtime GitHub notifications bot (Jun 26 2024 at 19:16):

fitzgen requested cfallin for a review on PR #8876.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 26 2024 at 19:16):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #8876.

view this post on Zulip 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 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.

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jun 26 2024 at 19:25):

fitzgen updated PR #8876.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2024 at 17:45):

cfallin created PR review comment:

A little thing but rather than the explicit + 4 here, could we put the push_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.

view this post on Zulip 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)

view this post on Zulip 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...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2024 at 17:45):

cfallin created PR review comment:

(likewise here as above)

view this post on Zulip 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 the BackwardInsnIndex type?

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2024 at 17:53):

fitzgen submitted PR review.

view this post on Zulip 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).

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2024 at 18:22):

fitzgen updated PR #8876.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2024 at 18:22):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2024 at 18:22):

fitzgen created PR review comment:

Done!

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2024 at 18:22):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2024 at 18:22):

fitzgen created PR review comment:

And done!

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2024 at 18:22):

fitzgen has enabled auto merge for PR #8876.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2024 at 18:23):

fitzgen commented on PR #8876:

Thanks for the review, Chris!

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2024 at 18:27):

fitzgen updated PR #8876.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2024 at 18:55):

fitzgen merged PR #8876.


Last updated: Nov 22 2024 at 16:03 UTC