Page MenuHomePhabricator

[lld][WebAssembly] Allow first thread to use static TLS region
ClosedPublic

Authored by sbc100 on May 20 2022, 4:51 PM.

Details

Summary

It turns out we were already allocating static address space for TLS
data along with the non-TLS static data, but this space was going
unused/ignored.

With this change, we include the TLS segment in __wasm_init_memory
(which does the work of loading the passive segments into memory when a
module is first loaded). We also set the __tls_base global to point
to the start of this segment.

This means that the runtime can use this static copy of the TLS data for
the first/primary thread if it chooses, rather than doing a runtime
allocation prior to calling __wasm_init_tls.

Practically speaking, this will allow emscripten to avoid dynamic
allocation of TLS region on the main thread.

Part of work towards https://github.com/emscripten-core/emscripten/issues/16948

Diff Detail

Event Timeline

sbc100 created this revision.May 20 2022, 4:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2022, 4:51 PM
Herald added subscribers: pmatos, asb, wingo and 5 others. · View Herald Transcript
sbc100 requested review of this revision.May 20 2022, 4:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2022, 4:51 PM
sbc100 edited the summary of this revision. (Show Details)May 20 2022, 4:52 PM
sbc100 added reviewers: dschuff, tlively.
sbc100 updated this revision to Diff 431099.May 20 2022, 5:01 PM
  • tweaks
tlively added inline comments.May 20 2022, 5:36 PM
lld/test/wasm/tls.s
109 ↗(On Diff #431099)

Where does this number come from?

lld/wasm/Writer.cpp
280

Why these changes?

973–975

Why did this change?

1212–1218

Is there only ever one TLS segment? It feels weird to have this in a loop.

1272–1273

Can you elaborate on this comment? I don't understand why the condition should be removed. Surely we need to continue to avoid doing data.drop on TLS segments?

sbc100 updated this revision to Diff 431252.May 22 2022, 1:14 PM
  • simplify
sbc100 edited the summary of this revision. (Show Details)May 22 2022, 1:14 PM
sbc100 edited the summary of this revision. (Show Details)
sbc100 retitled this revision from [lld][WebAssembly] Allow first-loader to use static TLS region to [lld][WebAssembly] Allow first thread to use static TLS region.

OK I simplified and refactored and re-wrote the description. Maybe it will be more clear what is going on here now?

lld/wasm/Writer.cpp
280

P

280

Previously __tls_base was not set at all (just set to zero) in the shared memory case, and we relied on the embedder always calling __wasm_tls_init to set it.

After this change the embedder only needs to call __wasm_tls_init on secondary threads.

In the primary thread we initialize __tls_basehere to the offset of the segment from __memory_base, then we call
__wasm_tls_init as part of __wasm_memory_init.. which adjusts it accordingly.

tlively accepted this revision.May 23 2022, 2:56 PM

Thanks, this makes more sense now. LGTM with fixes and extra information in that comment.

Also, in the third paragraph of the description:

"This means that the runtime and" => "This means that the runtime can"

lld/wasm/Writer.cpp
1195
This revision is now accepted and ready to land.May 23 2022, 2:56 PM
sbc100 edited the summary of this revision. (Show Details)May 23 2022, 5:18 PM
sbc100 updated this revision to Diff 431537.May 23 2022, 5:26 PM
sbc100 marked 2 inline comments as not done.
  • fix test
This revision was landed with ongoing or failed builds.May 23 2022, 5:27 PM
This revision was automatically updated to reflect the committed changes.