Stream: cranelift

Topic: safepoints at loop headers?

fitzgen (he/him) (Jun 03 2020 at 22:03):

right now, we only insert safepoints at calls and interrupt traps:

https://github.com/bytecodealliance/wasmtime/blob/master/cranelift/codegen/src/regalloc/safepoint.rs#L54-L61

it seems like we would really want to insert them at loop headers as well, to allow GC during long-lasting loops.

does anyone know if either (a) I'm overlooking some code that handles this for us, or (b) if we already have an issue on file for this (a cursory search failed to find any)? thanks!

Chris Fallin (Jun 03 2020 at 22:26):

FWIW, I did some digging last week on this (in the context of eventual reftype work) and also couldn't find anything for loops

fitzgen (he/him) (Jun 03 2020 at 23:03):

filed https://github.com/bytecodealliance/wasmtime/issues/1815

fitzgen (he/him) (Jun 03 2020 at 23:04):

but actually, now that I think about it, the loop header has to have some sort of check for whether the gc requested that the wasm synchronize and stop at a safepoint, which likely will involve some call, e.g it checks a flag hanging off the vmctx and (if set) calls into gc_synchronize or something, and that call will get a safepoint. so maybe everything is fine here, and nothing needs to change...

Chris Fallin (Jun 03 2020 at 23:33):

I was curious how this works in wasm on Ion/x86, so I went digging a bit... here's the insertion of an interrupt-check op in the loop header: https://searchfox.org/mozilla-central/source/js/src/wasm/WasmIonCompile.cpp#2218 (MWasmInterruptCheck IR node, lowered to LWasmInterruptCheck) which eventually leads to this codegen: https://searchfox.org/mozilla-central/source/js/src/jit/CodeGenerator.cpp#13952

Chris Fallin (Jun 03 2020 at 23:34):

The interrupt check itself is a conditional branch (on some flag in the tlsdata) around a trap instruction: https://searchfox.org/mozilla-central/source/js/src/jit/MacroAssembler.cpp#3253

Chris Fallin (Jun 03 2020 at 23:35):

interestingly, it does seem to add a safepoint in the happy (non-interrupt) path, so I can only assume this is zero cost (i.e. doesn't spill anything): https://searchfox.org/mozilla-central/source/js/src/jit/CodeGenerator.cpp#13958

Chris Fallin (Jun 03 2020 at 23:36):

(actually, that safepoint is right at the return PC for the trap so semantically it's really part of the cold path; I suppose because only the return PC is available when the trap is handled)

Chris Fallin (Jun 03 2020 at 23:36):

in any case you're probably right that we'd be able to call some handler and we'd get the safepoint for free

Benjamin Bouvier (Jun 04 2020 at 10:33):

So Cranelift-in-Spidermonkey generates an interrupt check as well, at the top of loop headers https://searchfox.org/mozilla-central/source/js/src/wasm/cranelift/src/wasm2clif.rs#1244, and this will end handling the interrupt the same way Spidermonkey does.

Benjamin Bouvier (Jun 04 2020 at 10:35):

The translate_loop_header documentation explicitly mentions safepoints, but it seems we don't generate them at this point :thinking:

Benjamin Bouvier (Jun 04 2020 at 13:32):

I just saw this link from @fitzgen (he/him) 's issue on github: https://github.com/bytecodealliance/wasmtime/blob/master/cranelift/codegen/src/regalloc/safepoint.rs#L54-L61
So yeah, since Spidermonkey generates this trapnz at loop headers, it will create a safepoint there. This sort of synchronization might be a bit fragile...

Benjamin Bouvier (Jun 04 2020 at 13:51):

(And here's a new piece of information: it actually doesn't get added, because Spidermonkey generates trapif, not trap! Did i say this was fragile...)

fitzgen (he/him) (Jun 04 2020 at 15:09):

Only resumable_trap get safepoints because otherwise nothing is considered live after the trap

Benjamin Bouvier (Jun 04 2020 at 15:12):

Traps in Spidermonkey are resumable, so there might be something going on here...

Benjamin Bouvier (Jun 04 2020 at 15:19):

Actually, the CLIR Trap and ReusableTrap would both get a safepoint created, only not instructions using the CondTrap format.

fitzgen (he/him) (Jun 04 2020 at 16:36):

if you look at insert_and_encode_safepoint, if there are no live references, then it skips the insertion of the safepoint, and for non-resumable traps (trap) there are always no live references. I was playing around with this a bit yesterday trying to make emit_stackmaps check if let Some(TrapCode::Interrupt) = pos.func.dfg[inst].trap_code() instead of explicitly only checking for InstructionFormat::Trap instructions

Last updated: Nov 22 2024 at 17:03 UTC