Bug 169773 - WebAssembly: add fallback to use pinned register to load/store state
Summary: WebAssembly: add fallback to use pinned register to load/store state
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
: 169815 (view as bug list)
Depends on: 169611
Blocks: 168264 170135
  Show dependency treegraph
 
Reported: 2017-03-16 11:56 PDT by Yusuke Suzuki
Modified: 2017-03-28 15:16 PDT (History)
8 users (show)

See Also:


Attachments
Patch (14.61 KB, patch)
2017-03-25 11:30 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (25.70 KB, patch)
2017-03-25 13:01 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (25.67 KB, patch)
2017-03-25 13:15 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (33.13 KB, patch)
2017-03-26 09:07 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (35.61 KB, patch)
2017-03-26 10:50 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (37.20 KB, patch)
2017-03-26 10:58 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (37.77 KB, patch)
2017-03-26 11:54 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (38.29 KB, patch)
2017-03-26 12:07 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (49.62 KB, patch)
2017-03-26 12:30 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (57.83 KB, patch)
2017-03-27 01:05 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (57.87 KB, patch)
2017-03-27 01:40 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (58.26 KB, patch)
2017-03-27 03:36 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (62.91 KB, patch)
2017-03-27 11:15 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (62.74 KB, patch)
2017-03-27 11:39 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (64.89 KB, patch)
2017-03-27 12:00 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (67.71 KB, patch)
2017-03-28 10:55 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (58.92 KB, patch)
2017-03-28 11:09 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (59.10 KB, patch)
2017-03-28 11:29 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-03-16 11:56:51 PDT
In Linux, we do not have any system reserved TLS slots.
Instead, we should store the Wasm state in pinned register.
Comment 1 Yusuke Suzuki 2017-03-16 12:47:24 PDT
Doc https://trac.webkit.org/changeset/207004
Comment 2 JF Bastien 2017-03-16 13:09:08 PDT
For now, platforms without fast TLS can't be PIC and won't be able to do structured cloning.
Comment 3 JF Bastien 2017-03-17 10:10:56 PDT
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 :)
Comment 4 JF Bastien 2017-03-17 10:56:07 PDT
*** Bug 169815 has been marked as a duplicate of this bug. ***
Comment 5 JF Bastien 2017-03-17 10:56:39 PDT
As mentioned in bug #169815 we probably want to do this unconditionally for ARM64.
Comment 6 Yusuke Suzuki 2017-03-25 11:30:35 PDT
Created attachment 305388 [details]
Patch

WIP
Comment 7 Yusuke Suzuki 2017-03-25 11:34:29 PDT
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.
Comment 8 Yusuke Suzuki 2017-03-25 13:01:24 PDT
Created attachment 305394 [details]
Patch

WIP
Comment 9 Yusuke Suzuki 2017-03-25 13:15:23 PDT
Created attachment 305395 [details]
Patch

WIP
Comment 10 Yusuke Suzuki 2017-03-26 09:07:17 PDT
Created attachment 305424 [details]
Patch

WIP
Comment 11 Yusuke Suzuki 2017-03-26 10:50:13 PDT
Created attachment 305426 [details]
Patch

WIP
Comment 12 Yusuke Suzuki 2017-03-26 10:58:41 PDT
Created attachment 305427 [details]
Patch

WIP
Comment 13 Yusuke Suzuki 2017-03-26 11:54:28 PDT
Created attachment 305429 [details]
Patch

WIP
Comment 14 Yusuke Suzuki 2017-03-26 12:07:27 PDT
Created attachment 305430 [details]
Patch

WIP
Comment 15 Yusuke Suzuki 2017-03-26 12:30:51 PDT
Created attachment 305432 [details]
Patch

WIP
Comment 16 Filip Pizlo 2017-03-26 13:48:11 PDT
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 17 Yusuke Suzuki 2017-03-26 14:36:23 PDT
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.
Comment 18 Yusuke Suzuki 2017-03-27 01:05:52 PDT
Created attachment 305453 [details]
Patch
Comment 19 Yusuke Suzuki 2017-03-27 01:40:25 PDT
Created attachment 305456 [details]
Patch
Comment 20 Yusuke Suzuki 2017-03-27 03:36:23 PDT
Created attachment 305460 [details]
Patch
Comment 21 JF Bastien 2017-03-27 09:12:18 PDT
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"?
Comment 22 Yusuke Suzuki 2017-03-27 11:15:38 PDT
Created attachment 305488 [details]
Patch
Comment 23 Yusuke Suzuki 2017-03-27 11:18:44 PDT
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.
Comment 24 Yusuke Suzuki 2017-03-27 11:39:18 PDT
Created attachment 305489 [details]
Patch
Comment 25 Yusuke Suzuki 2017-03-27 12:00:56 PDT
Created attachment 305491 [details]
Patch
Comment 26 Saam Barati 2017-03-27 13:56:51 PDT
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
Comment 27 Saam Barati 2017-03-27 13:57:45 PDT
(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)
Comment 28 Saam Barati 2017-03-28 00:03:14 PDT
(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 29 Saam Barati 2017-03-28 00:26:19 PDT
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 30 Yusuke Suzuki 2017-03-28 07:19:23 PDT
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 31 Saam Barati 2017-03-28 10:49:43 PDT
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.
Comment 32 Yusuke Suzuki 2017-03-28 10:55:08 PDT
Created attachment 305604 [details]
Patch
Comment 33 Saam Barati 2017-03-28 10:55:57 PDT
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 34 Yusuke Suzuki 2017-03-28 10:57:46 PDT
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 35 Yusuke Suzuki 2017-03-28 10:59:03 PDT
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.
Comment 36 Yusuke Suzuki 2017-03-28 11:09:37 PDT
Created attachment 305607 [details]
Patch
Comment 37 Saam Barati 2017-03-28 11:13:53 PDT
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 38 Yusuke Suzuki 2017-03-28 11:29:11 PDT
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.
Comment 39 Yusuke Suzuki 2017-03-28 11:29:41 PDT
Created attachment 305616 [details]
Patch
Comment 40 Saam Barati 2017-03-28 14:44:43 PDT
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 41 Yusuke Suzuki 2017-03-28 15:13:45 PDT
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.
Comment 42 Yusuke Suzuki 2017-03-28 15:16:10 PDT
Committed r214498: <http://trac.webkit.org/changeset/214498>