Bug 169773

Doc https://trac.webkit.org/changeset/207004

For now, platforms without fast TLS can't be PIC and won't be able to do structured cloning.

Clarifying my previous comment: if we don't address this bug then we can't structured clone. Using a pinned register would enable structured cloning. No rush since the MacOS build doesn't have structure cloning either, blocked on bug #168264 :)

*** Bug 169815 has been marked as a duplicate of this bug. ***

As mentioned in bug #169815 we probably want to do this unconditionally for ARM64.

Created attachment 305388 [details] Patch WIP

Comment on attachment 305388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305388&action=review > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1203 > + GPRReg topJSWebAssemblyInstancePointer = pinnedRegs.topJSWebAssemblyInstancePointer; > + jit.loadPtr(CCallHelpers::Address(GPRInfo::callFrameRegister, CallFrameSlot::callee * static_cast<int>(sizeof(Register))), topJSWebAssemblyInstancePointer); > + jit.andPtr(CCallHelpers::TrustedImmPtr(MarkedBlock::blockMask), topJSWebAssemblyInstancePointer); > + jit.loadPtr(CCallHelpers::Address(topJSWebAssemblyInstancePointer, MarkedBlock::offsetOfVM()), topJSWebAssemblyInstancePointer); > + jit.loadPtr(CCallHelpers::Address(topJSWebAssemblyInstancePointer, VM::wasmContextOffset()), topJSWebAssemblyInstancePointer); NOTE: Should load instance from callee. And we should remove loadWasmContext / storeWasmContext from non jit code. Instead, we should pass it to the C argument.

Created attachment 305394 [details] Patch WIP

Created attachment 305395 [details] Patch WIP

Created attachment 305424 [details] Patch WIP

Created attachment 305426 [details] Patch WIP

Created attachment 305427 [details] Patch WIP

Created attachment 305429 [details] Patch WIP

Created attachment 305430 [details] Patch WIP

Created attachment 305432 [details] Patch WIP

Comment on attachment 305432 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305432&action=review LGTM so far! > Source/JavaScriptCore/jit/AssemblyHelpers.cpp:782 > + move(Wasm::PinnedRegisterInfo::get().topJSWebAssemblyInstancePointer, dst); I think that we should stop using the "topBlahBlahPointer" terminology. "Context" is the usual VM term for this. That's why OSes say "context switch" rather than "top process switch". And OSes also have stacks of context, because interrupts. So, lets call this wasmContext rather than topJSWebAssemblyInstancePointer. Also, thank you for moving this out into the .cpp file! We should do more of that. > Source/JavaScriptCore/jit/AssemblyHelpers.cpp:793 > + move(src, Wasm::PinnedRegisterInfo::get().topJSWebAssemblyInstancePointer); Ditto. > Source/JavaScriptCore/jit/Repatch.cpp:583 > + // The WebAssembly -> JS stub sets it caller frame's callee to a WebAssemblyToJSCallee instance. > + return callee->inherits(vm, WebAssemblyToJSCallee::info()); Do we have enough bits in JSType to just give this a type? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:229 > + GPRReg m_topJSWebAssemblyInstancePointer; wasmContext! > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:248 > - return block->appendNew<Value>(proc, Identity, Origin(), patchpoint); > + return block->appendNew<ArgumentRegValue>(proc, Origin(), PinnedRegisterInfo::get().topJSWebAssemblyInstancePointer); It's interesting that ArgumentRegValue works for this. I guess it ought to just work if the Procedure never has anything that clobbers this register. It's correct because in the context of a Procedure, we can assume that this register is simply immutable. ArgumentRegValue is a valid way to read immutable pinned registers. Is this property documented anywhere? If not, can you document it, maybe even in the Websites/webkit.org/docs/b3 documentation? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1200 > + GPRReg topJSWebAssemblyInstancePointer = pinnedRegs.topJSWebAssemblyInstancePointer; wasmContext

Comment on attachment 305432 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305432&action=review Thanks! >> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:782 >> + move(Wasm::PinnedRegisterInfo::get().topJSWebAssemblyInstancePointer, dst); > > I think that we should stop using the "topBlahBlahPointer" terminology. "Context" is the usual VM term for this. That's why OSes say "context switch" rather than "top process switch". And OSes also have stacks of context, because interrupts. So, lets call this wasmContext rather than topJSWebAssemblyInstancePointer. > > Also, thank you for moving this out into the .cpp file! We should do more of that. Yeah, I'll rename it to WasmContext :) >> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:793 >> + move(src, Wasm::PinnedRegisterInfo::get().topJSWebAssemblyInstancePointer); > > Ditto. Fixed. >> Source/JavaScriptCore/jit/Repatch.cpp:583 >> + return callee->inherits(vm, WebAssemblyToJSCallee::info()); > > Do we have enough bits in JSType to just give this a type? I think we still have enough space! I'll assign a special JSType to accelerate this check :) >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:229 >> + GPRReg m_topJSWebAssemblyInstancePointer; > > wasmContext! Fixed. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:248 >> + return block->appendNew<ArgumentRegValue>(proc, Origin(), PinnedRegisterInfo::get().topJSWebAssemblyInstancePointer); > > It's interesting that ArgumentRegValue works for this. I guess it ought to just work if the Procedure never has anything that clobbers this register. It's correct because in the context of a Procedure, we can assume that this register is simply immutable. ArgumentRegValue is a valid way to read immutable pinned registers. > > Is this property documented anywhere? If not, can you document it, maybe even in the Websites/webkit.org/docs/b3 documentation? As usual, it is documented in the code comment :P (in B3Effects.h). Yeah, we should document it, I'll add some descriptions about immutable pinned register & ArgumentReg. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1200 >> + GPRReg topJSWebAssemblyInstancePointer = pinnedRegs.topJSWebAssemblyInstancePointer; > > wasmContext Fixed.

Created attachment 305453 [details] Patch

Created attachment 305456 [details] Patch

Created attachment 305460 [details] Patch

Comment on attachment 305460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305460&action=review lgtm, thanks! > Source/JavaScriptCore/heap/MarkedBlock.h:302 > + static ptrdiff_t offsetOfVM() constexpr variable instead. > Source/JavaScriptCore/runtime/VM.h:492 > + static ptrdiff_t wasmContextOffset() constexpr variable. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:234 > static Value* loadWasmContext(Procedure& proc, BasicBlock* block) Should we rename this and below from "load" and "store" since it can just get a pinned registers? Maybe "materialize" and "preserve" or something? Not a fan of preserve... > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:253 > + // We use pinned callee save register for this purpose. Thus, we do not need to restore the instane register on the caller side. "instane"?

Created attachment 305488 [details] Patch

Comment on attachment 305460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305460&action=review Thanks! Updated. >> Source/JavaScriptCore/heap/MarkedBlock.h:302 >> + static ptrdiff_t offsetOfVM() > > constexpr variable instead. Unfortunately, we cannot use it here because OBJECT_OFFSETOF is implemented for non-POD things and it uses reinterpret_cast. >> Source/JavaScriptCore/runtime/VM.h:492 >> + static ptrdiff_t wasmContextOffset() > > constexpr variable. Ditto. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:234 >> static Value* loadWasmContext(Procedure& proc, BasicBlock* block) > > Should we rename this and below from "load" and "store" since it can just get a pinned registers? Maybe "materialize" and "preserve" or something? Not a fan of preserve... Materialize sounds nice. Fixed. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:253 >> + // We use pinned callee save register for this purpose. Thus, we do not need to restore the instane register on the caller side. > > "instane"? Fixed.

Created attachment 305489 [details] Patch

Created attachment 305491 [details] Patch

Comment on attachment 305491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305491&action=review r- because we're moving to a PIC architecture. We should ensure that this patch doesn't add more things that need to be changed as we move Wasm runtime above VM. See comments below. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1235 > + // FIXME: JSWebAssemblyCallee should have a pointer to JSWebAssemblyInstance instead. > + GPRReg wasmContext = pinnedRegs.wasmContextPointer; > + if (!useFastTLS()) { > + jit.loadPtr(CCallHelpers::Address(GPRInfo::callFrameRegister, CallFrameSlot::callee * static_cast<int>(sizeof(Register))), wasmContext); > + jit.andPtr(CCallHelpers::TrustedImmPtr(MarkedBlock::blockMask), wasmContext); > + jit.loadPtr(CCallHelpers::Address(wasmContext, MarkedBlock::offsetOfVM()), wasmContext); > + jit.loadPtr(CCallHelpers::Address(wasmContext, VM::wasmContextOffset()), wasmContext); > + } This is going to make it harder to do PIC. We need a way to get the context outside the VM. Maybe the first argument to every JS wrapper can be a VM*? Or maybe there is another solution

(In reply to Saam Barati from comment #26) > Comment on attachment 305491 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=305491&action=review > > r- because we're moving to a PIC architecture. We should ensure that this > patch doesn't add more things that need to be changed as we move Wasm > runtime above VM. See comments below. > > > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1235 > > + // FIXME: JSWebAssemblyCallee should have a pointer to JSWebAssemblyInstance instead. > > + GPRReg wasmContext = pinnedRegs.wasmContextPointer; > > + if (!useFastTLS()) { > > + jit.loadPtr(CCallHelpers::Address(GPRInfo::callFrameRegister, CallFrameSlot::callee * static_cast<int>(sizeof(Register))), wasmContext); > > + jit.andPtr(CCallHelpers::TrustedImmPtr(MarkedBlock::blockMask), wasmContext); > > + jit.loadPtr(CCallHelpers::Address(wasmContext, MarkedBlock::offsetOfVM()), wasmContext); > > + jit.loadPtr(CCallHelpers::Address(wasmContext, VM::wasmContextOffset()), wasmContext); > > + } > > This is going to make it harder to do PIC. We need a way to get the context > outside the VM. Maybe the first argument to every JS wrapper can be a VM*? > Or maybe there is another solution (Obviously we would only do this when we're not using fast TLS)

(In reply to Saam Barati from comment #26) > Comment on attachment 305491 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=305491&action=review > > r- because we're moving to a PIC architecture. We should ensure that this > patch doesn't add more things that need to be changed as we move Wasm > runtime above VM. See comments below. > > > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1235 > > + // FIXME: JSWebAssemblyCallee should have a pointer to JSWebAssemblyInstance instead. > > + GPRReg wasmContext = pinnedRegs.wasmContextPointer; > > + if (!useFastTLS()) { > > + jit.loadPtr(CCallHelpers::Address(GPRInfo::callFrameRegister, CallFrameSlot::callee * static_cast<int>(sizeof(Register))), wasmContext); > > + jit.andPtr(CCallHelpers::TrustedImmPtr(MarkedBlock::blockMask), wasmContext); > > + jit.loadPtr(CCallHelpers::Address(wasmContext, MarkedBlock::offsetOfVM()), wasmContext); > > + jit.loadPtr(CCallHelpers::Address(wasmContext, VM::wasmContextOffset()), wasmContext); > > + } > > This is going to make it harder to do PIC. We need a way to get the context > outside the VM. Maybe the first argument to every JS wrapper can be a VM*? > Or maybe there is another solution Actually, this patch is good. It's definitely moving us in the right direction. Will review in a second. I didn't comprehend exactly what was going on the first time I read though it.

Comment on attachment 305491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305491&action=review Mostly LGTM, but I have some comments and questions. > Source/JavaScriptCore/runtime/JSCellInlines.h:352 > +inline bool isWebAssemblyToJSCallee(const JSCell* cell) > +{ > + return cell->type() == WebAssemblyToJSCalleeType; > +} Is this hot enough that it needs its own type? Why not just use inherits? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:246 > + [&] (CCallHelpers& jit, const StackmapGenerationParams& params) { This needs to be an [=] lambda. I saw today that somebody committed a [&] lambda as the patchpoint's generator. That's obviously wrong. Feel free to fix it if you want. If not, I will fix it in my patch. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:250 > + return block->appendNew<Value>(proc, Identity, Origin(), patchpoint); Why not just return patchpoint? Why return Identity(Patchpoint)? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:254 > + // FIXME: Because WasmToWasm call clobbers wasmContext register and does not restore it, we need to restore it in the caller side. > + // This prevents us from using ArgumentReg to this (logically) immutable pinned register. Why? I thought the semantics of ArgumentReg is that it's idempotent. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:259 > + patchpoint->setGenerator([&] (CCallHelpers&, const StackmapGenerationParams&) { }); ditto on lambda, just for style. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:269 > + patchpoint->append(ConstrainedValue(arg, ValueRep::SomeRegister)); Why? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:928 > + patchpoint->clobberLate(PinnedRegisterInfo::get().toSave()); Why is this being clobbered? These are callee saves, right? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:944 > + patchpoint->clobberLate(PinnedRegisterInfo::get().toSave()); Ditto. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:973 > + // Do not clobber pinned registers. Style nit: I don't think this comment is helpful > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1053 > + patchpoint->clobberLate(PinnedRegisterInfo::get().toSave()); ditto about why > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1234 > + GPRReg wasmContext = pinnedRegs.wasmContextPointer; > + if (!useFastTLS()) { > + jit.loadPtr(CCallHelpers::Address(GPRInfo::callFrameRegister, CallFrameSlot::callee * static_cast<int>(sizeof(Register))), wasmContext); > + jit.andPtr(CCallHelpers::TrustedImmPtr(MarkedBlock::blockMask), wasmContext); > + jit.loadPtr(CCallHelpers::Address(wasmContext, MarkedBlock::offsetOfVM()), wasmContext); > + jit.loadPtr(CCallHelpers::Address(wasmContext, VM::wasmContextOffset()), wasmContext); My comment on this code still holds. If we're not using fast TLS, it seems like we should have another way of getting the VM. I want to lift Callee above the VM, so perhaps we should pass VM* as a parameter when calling vmEntryToWasm and we're not using fast TLS. > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:104 > + visitor.append(thisObject->m_callee); Why did you move this off VM? > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:144 > + // FIXME: We would like to make loadWasmContext and storeWasmContext no-op if we use pinned registers. > + // However, we use VM.wasmContext field to pass instance to wasm function's JS glue code. > + // Once we start using usual WebAssemblyFunction as callee, we can retrieve instance from that. We don't want the callee to point to the instance. This won't work if we lift Callee above the VM. This also doesn't work today, since a module may have many instances. I think ultimately, we just don't want to rely on loadWasmContext and storeWasmContext when not using fast TLS. We should basically never have to emit those instructions, unless we're truly doing a context switch. > Websites/webkit.org/docs/b3/intermediate-representation.html:241 > + ArgumentRegValue class. It can be used to read a pinned register which is not clobbered > + inside the procedure.</dd> I don't think this needs any explanation w.r.t pinned registers. I'm pretty sure the semantics of ArgumentReg() is that it's idempotent within the execution of a function.

Comment on attachment 305491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305491&action=review Thanks >> Source/JavaScriptCore/runtime/JSCellInlines.h:352 >> +} > > Is this hot enough that it needs its own type? Why not just use inherits? It is called in linkFor operation. I think it should be fast. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:246 >> + [&] (CCallHelpers& jit, const StackmapGenerationParams& params) { > > This needs to be an [=] lambda. I saw today that somebody committed a [&] lambda as the patchpoint's generator. That's obviously wrong. Feel free to fix it if you want. If not, I will fix it in my patch. Oops, I'll fix it. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:250 >> + return block->appendNew<Value>(proc, Identity, Origin(), patchpoint); > > Why not just return patchpoint? Why return Identity(Patchpoint)? It is just the original code. Dropped. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:254 >> + // This prevents us from using ArgumentReg to this (logically) immutable pinned register. > > Why? I thought the semantics of ArgumentReg is that it's idempotent. Since WasmToWasm (different instance) call will clobber these registers and does not restore. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:259 >> + patchpoint->setGenerator([&] (CCallHelpers&, const StackmapGenerationParams&) { }); > > ditto on lambda, just for style. Fixed. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:269 >> + patchpoint->append(ConstrainedValue(arg, ValueRep::SomeRegister)); > > Why? Since the following code generator assumes that wasm context is in a register. And store it to the fast TLS slot since wasm to wasm call clobbers it. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:928 >> + patchpoint->clobberLate(PinnedRegisterInfo::get().toSave()); > > Why is this being clobbered? These are callee saves, right? Right. They are callee saves. But, wasm to wasm function calls clobber them and do not restore these registers. That's why we call restoreWebAssemblyGlobalState() currently. Changing this calling convention requires reasonable change. So, in this patch, we just use the current calling convension. And we annotate the clobber rules precisely. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:944 >> + patchpoint->clobberLate(PinnedRegisterInfo::get().toSave()); > > Ditto. The same reason. Current wasm to wasm function call clobbers them. This annotation precisely models the current calling convension. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:973 >> + // Do not clobber pinned registers. > > Style nit: I don't think this comment is helpful OK, dropped. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1053 >> + patchpoint->clobberLate(PinnedRegisterInfo::get().toSave()); > > ditto about why The same reason. Wasm to wasm function actually clobbers right now. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1234 >> + jit.loadPtr(CCallHelpers::Address(wasmContext, VM::wasmContextOffset()), wasmContext); > > My comment on this code still holds. If we're not using fast TLS, it seems like we should have another way of getting the VM. I want to lift Callee above the VM, so perhaps we should pass VM* as a parameter when calling vmEntryToWasm and we're not using fast TLS. I think passing JSWebAssemblyInstance (wasm context) directly is simpler. It changes the current vmEntryToWasm signature, and we need to decouple it from vmEntryToJS thing. I think this should be done in a separate patch. This patch is now focusing on removing direct VM address use in the code. At that time, we should reconsider the current wasm to wasm calling convention too. >> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:104 >> + visitor.append(thisObject->m_callee); > > Why did you move this off VM? Now, callee is per JSWebassemblyInstance. And it has a pointer to the instance. And this is used in linkFor function to get appropriate module. So, anyway, we need to decouple this callee one from VM. Maybe, this callee should point module instead of instance. I'll change this part. >> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:144 >> + // Once we start using usual WebAssemblyFunction as callee, we can retrieve instance from that. > > We don't want the callee to point to the instance. This won't work if we lift Callee above the VM. This also doesn't work today, since a module may have many instances. > I think ultimately, we just don't want to rely on loadWasmContext and storeWasmContext when not using fast TLS. We should basically never have to emit those instructions, > unless we're truly doing a context switch. Yup, right. I think we should pass this instance directly to vmEntryToWasm. Why I use this approach is now we still using vmEntryToJS. But I think it should be done in a separate patch as described. This patch is now focusing on removing direct reference to VM in the code. When this change is done, we can make loadWasmContext and storeWasmContext no-op in the environment not using fast TLS. >> Websites/webkit.org/docs/b3/intermediate-representation.html:241 >> + inside the procedure.</dd> > > I don't think this needs any explanation w.r.t pinned registers. I'm pretty sure the semantics of ArgumentReg() is that it's idempotent within the execution of a function. OK, dropped.

Comment on attachment 305491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305491&action=review >>> Source/JavaScriptCore/runtime/JSCellInlines.h:352 >>> +} >> >> Is this hot enough that it needs its own type? Why not just use inherits? > > It is called in linkFor operation. I think it should be fast. I think it'd be cleaner if we did something like this: we should have a JS and Wasm call IC linking function. They can both call into a common function that does most of the shared work. We have two versions of the wasm function, one if we're using fast TLS, and one if we're not. (Or we can just use the same one in both cases and pass in the context unconditionally, that's probably better). The wasm link thunk will expect the context in a particular register. That way, when it calls out to C code, it can pass it to the C call. I'm also happy to make this change in a follow up if we need to when I change callee to be above VM.

Created attachment 305604 [details] Patch

Comment on attachment 305491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305491&action=review >>>> Source/JavaScriptCore/runtime/JSCellInlines.h:352 >>>> +} >>> >>> Is this hot enough that it needs its own type? Why not just use inherits? >> >> It is called in linkFor operation. I think it should be fast. > > I think it'd be cleaner if we did something like this: we should have a JS and Wasm call IC linking function. They can both call into a common function that does most of the shared work. > We have two versions of the wasm function, one if we're using fast TLS, and one if we're not. (Or we can just use the same one in both cases and pass in the context unconditionally, that's probably better). > The wasm link thunk will expect the context in a particular register. That way, when it calls out to C code, it can pass it to the C call. > > I'm also happy to make this change in a follow up if we need to when I change callee to be above VM. It's probably best to do this in a follow up. I'll do this after your patch lands: https://bugs.webkit.org/show_bug.cgi?id=170185 >>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:254 >>> + // This prevents us from using ArgumentReg to this (logically) immutable pinned register. >> >> Why? I thought the semantics of ArgumentReg is that it's idempotent. > > Since WasmToWasm (different instance) call will clobber these registers and does not restore. I'm still confused why ArgumentReg won't work here. I thought the semantics of ArgumentReg were that it's idempotent. >>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:269 >>> + patchpoint->append(ConstrainedValue(arg, ValueRep::SomeRegister)); >> >> Why? > > Since the following code generator assumes that wasm context is in a register. And store it to the fast TLS slot since wasm to wasm call clobbers it. Oops, I misread this code. Ignore my comment. > Source/JavaScriptCore/wasm/WasmContext.h:-38 > -namespace JSC { > - > -class JSWebAssemblyInstance; > -class VM; > - > -JSWebAssemblyInstance* loadWasmContext(VM&); > -void storeWasmContext(VM&, JSWebAssemblyInstance*); > - > -} // namespace JSC Lets keep something called WasmContext. Maybe we can just typedef it to be an instance for now. And then we could have functions on the context that get us the state we need. If we keep something called WasmContext, we keep our options open for easily changing it to be something else in the future.

Comment on attachment 305491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305491&action=review >>>>> Source/JavaScriptCore/runtime/JSCellInlines.h:352 >>>>> +} >>>> >>>> Is this hot enough that it needs its own type? Why not just use inherits? >>> >>> It is called in linkFor operation. I think it should be fast. >> >> I think it'd be cleaner if we did something like this: we should have a JS and Wasm call IC linking function. They can both call into a common function that does most of the shared work. >> We have two versions of the wasm function, one if we're using fast TLS, and one if we're not. (Or we can just use the same one in both cases and pass in the context unconditionally, that's probably better). >> The wasm link thunk will expect the context in a particular register. That way, when it calls out to C code, it can pass it to the C call. >> >> I'm also happy to make this change in a follow up if we need to when I change callee to be above VM. > > It's probably best to do this in a follow up. > > I'll do this after your patch lands: > https://bugs.webkit.org/show_bug.cgi?id=170185 I've just updated the patch, and read this comment right now. Agreed.

Comment on attachment 305491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305491&action=review >> Source/JavaScriptCore/wasm/WasmContext.h:-38 >> -} // namespace JSC > > Lets keep something called WasmContext. Maybe we can just typedef it to be an instance for now. > And then we could have functions on the context that get us the state we need. If we keep something > called WasmContext, we keep our options open for easily changing it to be something else in the future. OK, I'll revert this dropping part.

Created attachment 305607 [details] Patch

Comment on attachment 305607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305607&action=review Patch LGTM, just one suggestion > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:241 > + if (useFastTLS()) { Can we not tie the context being pinned vs being in TLS just to "useFastTLS". Can we maybe have this tied to something like: "useFastTLSForContext", that way, we can change context independent of other things in the future. For example, we might want to pin context on arm64, but not x86 even if FAST_TLS is enabled.

Comment on attachment 305607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305607&action=review >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:241 >> + if (useFastTLS()) { > > Can we not tie the context being pinned vs being in TLS just to "useFastTLS". Can we maybe have this tied to something like: > "useFastTLSForContext", that way, we can change context independent of other things in the future. For example, we might want > to pin context on arm64, but not x86 even if FAST_TLS is enabled. Sounds nice.

Created attachment 305616 [details] Patch

Comment on attachment 305616 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305616&action=review r=me > Source/JavaScriptCore/jit/AssemblyHelpers.cpp:812 > +#if ENABLE(WEBASSEMBLY) > +void AssemblyHelpers::loadWasmContext(GPRReg dst) > +{ > +#if ENABLE(FAST_TLS_JIT) > + if (Options::useWebAssemblyFastTLS()) { > + loadFromTLSPtr(fastTLSOffsetForKey(WTF_WASM_CONTEXT_KEY), dst); > + return; > + } > +#endif > + move(Wasm::PinnedRegisterInfo::get().wasmContextPointer, dst); > +} > + > +void AssemblyHelpers::storeWasmContext(GPRReg src) > +{ > +#if ENABLE(FAST_TLS_JIT) > + if (Options::useWebAssemblyFastTLS()) { > + storeToTLSPtr(src, fastTLSOffsetForKey(WTF_WASM_CONTEXT_KEY)); > + return; > + } > +#endif > + move(src, Wasm::PinnedRegisterInfo::get().wasmContextPointer); > +} > + > +bool AssemblyHelpers::loadWasmContextNeedsMacroScratchRegister() > +{ > +#if ENABLE(FAST_TLS_JIT) > + if (Options::useWebAssemblyFastTLS()) > + return loadFromTLSPtrNeedsMacroScratchRegister(); > +#endif > + return false; > +} > + > +bool AssemblyHelpers::storeWasmContextNeedsMacroScratchRegister() > +{ > +#if ENABLE(FAST_TLS_JIT) > + if (Options::useWebAssemblyFastTLS()) > + return storeToTLSPtrNeedsMacroScratchRegister(); > +#endif > + return false; > +} These all should all interface with "useFastTLSForContext" instead of how they're written. > Source/JavaScriptCore/wasm/WasmMemoryInformation.h:98 > +inline bool useFastTLSForContext() Style nit: Can we call this "useFastTLSForWasmContext"?

Comment on attachment 305616 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305616&action=review Thanks, I'm going to land it. >> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:812 >> +} > > These all should all interface with "useFastTLSForContext" instead of how they're written. Ah, right. Fixed. >> Source/JavaScriptCore/wasm/WasmMemoryInformation.h:98 >> +inline bool useFastTLSForContext() > > Style nit: Can we call this "useFastTLSForWasmContext"? Thanks, fixed.

Committed r214498: <http://trac.webkit.org/changeset/214498>