Closed Bug 1288944 Opened 4 years ago Closed 4 years ago

Baldr: switch to callee's pinned/TLS registers at call_indirect

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(12 files, 2 obsolete files)

36.79 KB, patch
jolesen
: review+
Details | Diff | Splinter Review
1.64 KB, patch
jolesen
: review+
Details | Diff | Splinter Review
36.39 KB, patch
jolesen
: review+
Details | Diff | Splinter Review
5.83 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
36.00 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
19.04 KB, patch
jolesen
: review+
Details | Diff | Splinter Review
8.66 KB, patch
jolesen
: review+
Details | Diff | Splinter Review
6.10 KB, patch
jolesen
: review+
Details | Diff | Splinter Review
18.91 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
27.53 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
22.46 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
4.90 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
When a wasm module's table is imported or exported, a call_indirect can change instance and so the TLS and any pinned (heap and global) registers need to be switched before calling the callee and restored after the callee returns.
Attached patch tls-instance-ptr (obsolete) — Splinter Review
This patch moves the Instance* to TlsData and makes WasmTlsReg available in import stubs.
Attachment #8776663 - Flags: review?(jolesen)
Attached patch tls-instance-ptrSplinter Review
Oops, forgot to qref.
Attachment #8776663 - Attachment is obsolete: true
Attachment #8776663 - Flags: review?(jolesen)
Attachment #8776664 - Flags: review?(jolesen)
Comment on attachment 8776664 [details] [diff] [review]
tls-instance-ptr

Review of attachment 8776664 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/arm/Lowering-arm.cpp
@@ +837,5 @@
>          LAsmJSCompareExchangeCallout* lir =
>              new(alloc()) LAsmJSCompareExchangeCallout(useRegisterAtStart(base),
>                                                        useRegisterAtStart(ins->oldValue()),
>                                                        useRegisterAtStart(ins->newValue()),
> +                                                      useFixed(ins->tls(), WasmTlsReg),

Does this argument actually have to be pinned to WasmTlsReg, or could it be a normal register argument?

@@ +885,5 @@
>      if (byteSize(ins->accessType()) != 4 && !HasLDSTREXBHD()) {
>          LAsmJSAtomicBinopCallout* lir =
>              new(alloc()) LAsmJSAtomicBinopCallout(useRegisterAtStart(base),
>                                                    useRegisterAtStart(ins->value()),
> +                                                  useFixed(ins->tls(), WasmTlsReg),

Same here, does this need to be useFixed?

::: js/src/jscntxt.h
@@ +216,5 @@
>      void* runtimeAddressOfInterruptUint32() { return runtime_->addressOfInterruptUint32(); }
>      void* stackLimitAddress(StackKind kind) { return &runtime_->mainThread.nativeStackLimit[kind]; }
>      void* stackLimitAddressForJitCode(StackKind kind);
>      uintptr_t stackLimit(StackKind kind) { return runtime_->mainThread.nativeStackLimit[kind]; }
> +    uintptr_t stackLimitForJitCode(StackKind kind);

Is this new function used anywhere? The TlsData is initialized from stackLimitADDRESSForJitCode().
Attachment #8776664 - Flags: review?(jolesen) → review+
(In reply to Jakob Stoklund Olesen [:jolesen] from comment #3)
> Does this argument actually have to be pinned to WasmTlsReg, or could it be
> a normal register argument?

Oops, nope, I suppose not.

> > +    uintptr_t stackLimitForJitCode(StackKind kind);
> 
> Is this new function used anywhere? The TlsData is initialized from
> stackLimitADDRESSForJitCode().

Oops, I split this out from a bigger patch and had meant to.
(In reply to Luke Wagner [:luke] from comment #4)
> > Does this argument actually have to be pinned to WasmTlsReg, or could it be
> > a normal register argument?
> 
> Oops, nope, I suppose not.

Actually, attempting to relax to useRegisterAtStart() hits this strange limitation:
  http://hg.mozilla.org/mozilla-central/file/tip/js/src/jit/BacktrackingAllocator.cpp
and switching to useRegister() hits this:
  http://hg.mozilla.org/mozilla-central/file/tip/js/src/jit/BacktrackingAllocator.cpp#l661
so apparently useFixed() is what I must do.
The next patches put a bit more pressure on TLS in a way that exposed a wasm/i64-only bug.
Attachment #8776742 - Flags: review?(jolesen)
Attached patch tls-context-ptrSplinter Review
This patch moves the heap/global/cx pointers into TLS and reloads these from TLS after import calls.  The plan is for subsequent patches to build on this by moving the pinned-reg-reloading to the caller.
Attachment #8776744 - Flags: review?(jolesen)
Comment on attachment 8776742 [details] [diff] [review]
fix-tls-i64-return

Review of attachment 8776742 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Attachment #8776742 - Flags: review?(jolesen) → review+
Comment on attachment 8776744 [details] [diff] [review]
tls-context-ptr

Review of attachment 8776744 [details] [diff] [review]:
-----------------------------------------------------------------

Very cool.

::: js/src/jit/none/MacroAssembler-none.h
@@ +405,5 @@
>  
>      void buildFakeExitFrame(Register, uint32_t*) { MOZ_CRASH(); }
>      bool buildOOLFakeExitFrame(void*) { MOZ_CRASH(); }
>      void loadWasmGlobalPtr(uint32_t, Register) { MOZ_CRASH(); }
> +    void loadWasmActivationFromTls(egister) { MOZ_CRASH(); }

"egister"?
Attachment #8776744 - Flags: review?(jolesen) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4b528711d2f
Baldr: fix TLS propagation in i64 case (r=jolesen)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5bb9552230c
Baldr: move the Instance* into TlsData (r=jolesen)
https://hg.mozilla.org/integration/mozilla-inbound/rev/04481dedde66
Baldr: move the JSContext* and memory* into TlsData (r=jolesen)
Whiteboard: [leave open]
Depends on: 1291757
Attached patch tweak-jit-exitSplinter Review
Slight optimization+simplification I noticed while here.
Attachment #8777423 - Flags: review?(bbouvier)
Attached patch rename-exitSplinter Review
"Exit" is ancient asm.js terminology.  What's clear now is that, when the dust settles, these "FuncImportExit"s are what will go in TlsData.  For now they're in global data, but global data is also currently thread-local, so I think the best name (for now and later) is FuncImportTls.
Attachment #8777424 - Flags: review?(bbouvier)
This patch preserves WasmTlsReg and pinned regs at import callsites.  Considering the previous patch, which also does saves TLS in JitExitStub, this feels redundant.  However:
 - the next patch allows imports to call exported wasm functions directly (no stub), so this *has* to be at the callsite
 - the JIT stub only has to save TLS because it uses the profiling prologue/epilogue (which in turn uses WasmTlsReg) and this is only because the wasm->JIT path is not fully optimized; the plan is to do a lot less work including not needing a prologue/epilogue so this will go away.
Attachment #8777431 - Flags: review?(jolesen)
Comment on attachment 8777431 [details] [diff] [review]
preserve-tls-at-import-calls

Review of attachment 8777431 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/WasmIonCompile.cpp
@@ +1044,5 @@
>              *def = nullptr;
>              return true;
>          }
>  
> +        auto callee = MWasmCall::Callee::import(globalDataOffset, call.tlsStackOffset_);

Either here, or in import() there should be an assertion that tlsStackOffset has actually been set.
Attachment #8777431 - Flags: review?(jolesen) → review+
(In reply to Jakob Stoklund Olesen [:jolesen] from comment #15)
> Either here, or in import() there should be an assertion that tlsStackOffset
> has actually been set.

That's really weird; I totally remember writing this assertion here, but it seems to have disappeared.  Good catch!
Depends on: 1291887
Actually, I realized that now all calls preserve TLS (for the reasons document in isCallPreserved()) so we can just remove that enum.
Attachment #8777561 - Flags: review?(jolesen)
Comment on attachment 8777561 [details] [diff] [review]
all-calls-preserve-tls

Review of attachment 8777561 [details] [diff] [review]:
-----------------------------------------------------------------

Nice
Attachment #8777561 - Flags: review?(jolesen) → review+
Comment on attachment 8777423 [details] [diff] [review]
tweak-jit-exit

Review of attachment 8777423 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks.

Why can't our compiler tell us when a variable is unused, at least in debug mode...
Attachment #8777423 - Flags: review?(bbouvier) → review+
Comment on attachment 8777424 [details] [diff] [review]
rename-exit

Review of attachment 8777424 [details] [diff] [review]:
-----------------------------------------------------------------

Not too big a fan of the new name, because knowing where the data is doesn't really help understanding what's within, in my opinion. However, I really can't find any better name.

Having "exit" was nice because:
1. we know it was related to the specific information needed when calling into the import (the JSFunction, the exit offset)
2. there was a nice symmetry between an Entry (calling into asm.js) and an Exit (calling out of asm.js). We still have the concept of Entry...

Yeah, I don't have a very strong opinion here. Looks good to me. The CallCompileState is really nice.

::: js/src/asmjs/WasmCode.h
@@ +175,5 @@
>  class FuncImport
>  {
>      Sig sig_;
>      struct CacheablePod {
> +        uint32_t tlsGlobalDataOffset_;

It's weird, because in the future global data will be global to all threads, right? so tlsGlobalData won't make sense at this time. Until then, it sounds fine to have this naming; but maybe it's just data associated to TLS (not "global"), so what do you think of tlsDataOffset_?

@@ +182,5 @@
>      } pod;
>  
>    public:
>      FuncImport() {
>        memset(&pod, 0, sizeof(CacheablePod));

pre-existing: 4 spaces indent wanted here

::: js/src/asmjs/WasmIonCompile.cpp
@@ +52,5 @@
> +class FunctionCompiler;
> +
> +// CallCompileState describes a call that is being compiled. Due to expression
> +// nesting, multiple calls can be in the middle of compilation at the same time
> +// and these are tracked in a stack by FunctionCompiler.

Sweet!
Attachment #8777424 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #20)
Another way to think of it is that, in the final state, there will be one FuncImport (in the metadata) and N FuncImportTls (one per thread accessing that FuncImport), so you can think of 
  FuncImportTls ≡ FuncImport × thread
which hopefully makes the name sound a bit better.

> > +        uint32_t tlsGlobalDataOffset_;
> 
> It's weird, because in the future global data will be global to all threads,
> right? so tlsGlobalData won't make sense at this time. Until then, it sounds
> fine to have this naming; but maybe it's just data associated to TLS (not
> "global"), so what do you think of tlsDataOffset_?

Yeah, it does sound like a contradiction.  "tls" is actually referring not to the fact that this is *in* TLS, but, rather, that this is the offset of the importee's TlsData*.  The plan is to fold today's global data into TlsData, at which point we would go and strip all the "Global" from all the *GlobalDataOffset names.  So this would become tlsDataOffset so I might as well do that now as you suggest.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9b4cd4644d2
Baldr: optimize JIT exit (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf99be5ca22
Baldr: rename CallArgs to CallCompileState and FuncImportExit to FuncImportTls (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/575b04a64ab8
Baldr: preserve TLS and pinned regs at call_import (r=jolesen)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ae1861ac1f
Baldr: mark all calls as preserving the TLS reg (r=jolesen)
For table calls, we simply know we're in a call and we clobber a non-ABI-arg reg.  Instead of doing this Temp funny business, this patch just uses an ABINonArgReg.  This ends up simplifying the next-next patch which factors out more code between Ion and Baseline.
Attachment #8778088 - Flags: review?(jolesen)
This patch simply moves MWasmCall::Callee out into WasmTypes.h where it is renamed wasm::CalleeDesc.  This is a prerequisite for the factoring in the next patch.
Attachment #8778089 - Flags: review?(bbouvier)
Attached patch factor-import-indirect-code (obsolete) — Splinter Review
De-duplication, especially nice given the next patch.
Attachment #8778090 - Flags: review?(bbouvier)
The next patch will switch TLS at call_indirect callsites.  Doing this requires each Table element to have its TLS available for efficient loading.  But we *already* store a Vector of WasmInstanceObject* in the WasmTableObject which is almost the same thing.  So as preparation for the next patch, this patch kilsl the WasmInstanceObject* Vector in WasmTableObject and instead moves the instance pointer into the wasm::Table, stored next to the callee*.  Turns out it was way simpler to do it this way :)
Attachment #8778091 - Flags: review?(bbouvier)
Actually, there is a much nicer way to refactor code that allows sharing the TLS-saving/restoring code with a later patch.
Attachment #8778090 - Attachment is obsolete: true
Attachment #8778090 - Flags: review?(bbouvier)
Attachment #8778099 - Flags: review?(bbouvier)
With previous refactorings in place, this is quite easy.
Attachment #8778110 - Flags: review?(bbouvier)
Comment on attachment 8778089 [details] [diff] [review]
factor-call-table-desc

Review of attachment 8778089 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: js/src/asmjs/WasmTypes.h
@@ +1114,5 @@
> +        c.which_ = AsmJSTable;
> +        c.u.table.desc_ = desc;
> +        return c;
> +    }
> +    explicit CalleeDesc(SymbolicAddress callee) : which_(Builtin) {

static CalleeDesc builtin(SymbolicAddress callee), for symmetry with others?
Attachment #8778089 - Flags: review?(bbouvier) → review+
Comment on attachment 8778099 [details] [diff] [review]
factor-import-indirect-code

Review of attachment 8778099 [details] [diff] [review]:
-----------------------------------------------------------------

Sweet

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +2107,4 @@
>          } else {
> +            const TableDesc& table = mg_.tables[0];
> +            MOZ_ASSERT(sig.id.kind() != SigIdDesc::Kind::None);
> +            MOZ_ASSERT(mg_.tables.length() == 1);

Can this assert go just above the def of table?

::: js/src/jit/MIR.h
@@ +13517,5 @@
>      }
>      const wasm::CalleeDesc &callee() const {
>          return callee_;
>      }
>      size_t spIncrement() const {

Can you make it return uint32_t now?

::: js/src/jit/MacroAssembler.cpp
@@ +2716,5 @@
> +void
> +MacroAssembler::wasmCallImport(const wasm::CallSiteDesc& desc, const wasm::CalleeDesc& callee)
> +{
> +    // Load the callee, before the caller's registers are clobbered.
> +    uint32_t globalDataOffset = callee.importGlobalDataOffset();;

pre-existing ubernit: ;;
Attachment #8778099 - Flags: review?(bbouvier) → review+
Comment on attachment 8778091 [details] [diff] [review]
move-instance-into-table

Review of attachment 8778091 [details] [diff] [review]:
-----------------------------------------------------------------

Great

::: js/src/jit/MacroAssembler.cpp
@@ +2767,5 @@
> +    // Load the callee from the table. The elements of an external table are
> +    // two words each.
> +    if (callee.wasmTableIsExternal()) {
> +        static_assert(sizeof(wasm::ExternalTableElem) == 8 ||
> +                      sizeof(wasm::ExternalTableElem) == 16, "");

Please put a comment in the static_assert.

How about:

static_assert(sizeof(wasm::ExternalTableElem) == 2 * sizeof(void*), "must be two words each");

and then you can remove that part from the comment just above?
Attachment #8778091 - Flags: review?(bbouvier) → review+
Comment on attachment 8778110 [details] [diff] [review]
swap-tls-at-call-indirect

Review of attachment 8778110 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!
Attachment #8778110 - Flags: review?(bbouvier) → review+
Comment on attachment 8778088 [details] [diff] [review]
simplify-call-import

Review of attachment 8778088 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +1525,5 @@
>  
>          // Switch to the callee's TLS and pinned registers and make the call.
>          masm.loadWasmGlobalPtr(globalDataOffset + offsetof(wasm::FuncImportTls, tls), WasmTlsReg);
>          masm.loadWasmPinnedRegsFromTls();
> +        masm.call(mir->desc(), ABINonArgReg0);

assert(WasmTlsReg != ABINonArgReg0) ?

The constraints between these different register classes is not obvious, so assertions are in order, I think.
Attachment #8778088 - Flags: review?(jolesen) → review+
(In reply to Jakob Stoklund Olesen [:jolesen] from comment #34)
> The constraints between these different register classes is not obvious, so
> assertions are in order, I think.

Agreed on adding the assert to make it locally clear.  However, you did write this constraint in the comments for all the WasmTlsReg definitions.  I was thinking it'd be good to move the WasmTlsReg down to be right under the ABINonArg* definitions, just like the WasmTableCall* regs which have a similar constraint.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d97a1da36f40
Baldr: change how wasm::Instance is traced (r=terrence)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f028ac66f96
Baldr: remove unnecessary temp register reservation in import calls (r=jolesen)
https://hg.mozilla.org/integration/mozilla-inbound/rev/71d0aa664ad1
Baldr: factor out MWasmCall::Callee and TableDesc (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe89cf4e4a9e
Baldr: factor out indirect/import call code into MacroAssembler (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9fe9e58523b
Baldr: fuse the WasmTableObject InstanceVector into the Table's array (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bbdeb042ad2
Baldr: change to and from callee's TLS at indirect callsite (r=bbouvier)
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.