Stream: cranelift

Topic: reading/writing fields of object pointed to by r32/r64

fitzgen (he/him) (Jun 24 2020 at 23:47):

Would it be better to

1. add encodings for load.r64 and store.r64 and fill_nop.r64 etc, or
2. to do a raw_bitcast from r64 to i64 and then use that bitcast'd value as the pointer for subsequent loads/stores?

Using raw_bitcast is leading to errors like "error: inst13: v13 is a real GPR value defined by a ghost instruction", while adding encodings seems like it touches a lot more stuff. I haven't got either approach working yet. I'm also a little wary that approach (2) might mess up stack maps generation.

Any suggestions as to which is the better approach?

cc @Dan Gohman @Benjamin Bouvier

Dan Gohman (Jun 24 2020 at 23:48):

When we do a load or store, do we keep track of the memory location for stackmaps?

fitzgen (he/him) (Jun 24 2020 at 23:49):

@Dan Gohman can you clarify a bit more? do you mean do we insert safepoints for loads/stores?

Dan Gohman (Jun 24 2020 at 23:50):

When we do a store, do we remember the memory location as a root that a GC would want to scan from?

fitzgen (he/him) (Jun 24 2020 at 23:52):

the way that stack maps work is we ask the register allocator which variables are live after a safepoint, and filter that set for values which are reference types. If the r64 is not used again after the bitcast, the register allocator will presumably believe it to be dead, and we won't include it in stack maps. The results of the bitcast will never be included in stack maps.

fitzgen (he/him) (Jun 24 2020 at 23:52):

this is making me think that adding load.r64 etc encodings is the most promising course of action

Dan Gohman (Jun 24 2020 at 23:53):

Yeah. Also, we use these load.64 and store.64 for accessing wasm tables, right?

Dan Gohman (Jun 24 2020 at 23:54):

and in the future, GC struct fields and so on

fitzgen (he/him) (Jun 24 2020 at 23:54):

yeah, that's what I am using them for now

Dan Gohman (Jun 24 2020 at 23:54):

where Cranelift itself isn't in charge of identifying the roots; the roots come from the wasm runtime which knows where all the tables are

fitzgen (he/him) (Jun 24 2020 at 23:54):

implementing the inline fast path for table.get and table.set

Chris Fallin (Jun 24 2020 at 23:55):

From the POV of embeddings like SpiderMonkey, they should definitely be separate instructions, I think -- a store of a GC'd pointer sometimes involves other logic (adding to a write buffer during incremental GC in progress, etc) so we want to be able to distinguish that from an i64

Dan Gohman (Jun 24 2020 at 23:55):

yeah, so then adding encodings for load.64 and store.r64 makes sense to me.

fitzgen (he/him) (Jun 24 2020 at 23:58):

@Dan Gohman great, thanks!

@Chris Fallin I have barriers here as well, I am expanding them inline in FuncEnvironment::translate_table_{get,set}. This should work well until sometime in the (fairly distant) future when we want to do barrier coalescing. See https://github.com/bytecodealliance/wasmtime/issues/1146#issuecomment-566790342 for details on the design space of when/where we handle barriers.

Chris Fallin (Jun 24 2020 at 23:59):

oh neat, I'll go do my homework -- thanks :-)

fitzgen (he/him) (Jun 25 2020 at 00:00):

(sorry, wasn't trying to be trite, I just did a Whole Write Up on that subject and might as well link it rather than poorly reproduce it here :-p )

Chris Fallin (Jun 25 2020 at 00:06):

No worries, happy to see all of this thought out already; thanks for the pointer!

fitzgen (he/him) (Jun 25 2020 at 18:44):

ok, these changes are part of https://github.com/bytecodealliance/wasmtime/pull/1923 and I've flagged you, @Dan Gohman, for review (feel free to redirect, if someone else makes more sense)

@Chris Fallin if by chance you're interested in what the GC barrier implementation looks like, they're in that PR too (inside the func_environ.rs file)

Dan Gohman (Jun 25 2020 at 19:03):

Is this PR branched off of the wiggle integration PR? It looks like there are a lot of unrelated commits

Alex Crichton (Jun 25 2020 at 19:04):

I think I may have messed that up by accident, a rebase should make those go away

fitzgen (he/him) (Jun 25 2020 at 19:40):

Yeah, just the last commit is relevant. I’ll rebase when I’m done with lunch

Last updated: Dec 23 2024 at 12:05 UTC