Page MenuHomePhabricator

[WebAssembly] Compile all TLS on Emscripten as local-exec
ClosedPublic

Authored by quantum on Jul 15 2019, 3:20 PM.

Details

Summary

Currently, on Emscripten, dynamic linking is not supported with threads.
This means that if thread-local storage is used, it must be used in a
statically-linked executable. Hence, local-exec is the only possible model.

This diff compiles all TLS variables to use local-exec on Emscripten as a
temporary measure until dynamic linking is supported with threads.

The goal for this is to allow C++ types with constructors to be thread-local.

Currently, when clang compiles a thread_local variable with a constructor,
it generates __tls_guard variable:

@__tls_guard = internal thread_local global i8 0, align 1

As no TLS model is specified, this is treated as general-dynamic, which we do
not support (and cannot support without implementing dynamic linking support
with threads in Emscripten). As a result, any C++ constructor in thread_local
variables would not compile.

By compiling all thread_local as local-exec, __tls_guard will compile and
we can support C++ constructors with TLS without implementing dynamic linking
with threads.

Depends on D64537

Diff Detail

Repository
rL LLVM

Event Timeline

quantum created this revision.Jul 15 2019, 3:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2019, 3:20 PM
aheejin added a comment.EditedJul 15 2019, 4:27 PM
  • Not sure what this means. The CL description says it tries to compile things into local-exec, but what this CL does look like checking if a global var is local-exec and if not report an error.
  • What is supposed to happen when the OS is not emscripten? If we don't support TLS in non-emscripten OSes regardless of its mode (local-exec or not), should we print an error in that case too?
  • Also, a test would be good too.
llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
184 ↗(On Diff #209970)

If this condition is to be removed once emscripten support dynamic linking with threads, rather than XXX, how about marking as TODO Remove this condition once emscripten supports dynamic linking with threads or something?

quantum updated this revision to Diff 209996.Jul 15 2019, 4:49 PM

Change to use TODO instead of XXX

Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2019, 4:49 PM
quantum updated this revision to Diff 209997.Jul 15 2019, 4:50 PM

Undo compressing the previous diff.

Harbormaster completed remote builds in B35057: Diff 209997.
quantum removed a project: Restricted Project.Jul 15 2019, 4:50 PM
quantum marked 2 inline comments as done.
quantum removed subscribers: jfb, cfe-commits.
quantum added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
184 ↗(On Diff #209970)

Done.

This comment was removed by aheejin.
quantum updated this revision to Diff 210137.Jul 16 2019, 11:12 AM
quantum marked an inline comment as done.
  • Not sure what this means. The CL description says it tries to compile things into local-exec, but what this CL does look like checking if a global var is local-exec and if not report an error.
  • What is supposed to happen when the OS is not emscripten? If we don't support TLS in non-emscripten OSes regardless of its mode (local-exec or not), should we print an error in that case too?
  • Also, a test would be good too.
  • On Emscripten, the code does not report an error if the variable is not local-exec and instead compiles it as local-exec.
  • The local-exec implementation should still theoretically work for other embeddings. Only the idea that only local-exec is currently possible is emscripten-specific.
  • A test is added.
quantum updated this revision to Diff 210168.Jul 16 2019, 1:52 PM

Rebase onto latest D64537

aheejin added inline comments.Jul 16 2019, 1:52 PM
llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
191 ↗(On Diff #210137)

So does this mean:

  1. If this is emscripten, convert all non-local-exec to local-exec
  2. If this is not emscripten, error on non-local exec

Is this right?

Anyhow, it might be better to tell what global variable is causing the problem to users in the error message, something like

report_fatal_error("only -ftls-model=local-exec is supported for now in non-Emscripten OSes: " + GA->getGlobal()->getName());
llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll
1 ↗(On Diff #210137)

Do we need a separate copy of tls.ll for this test? The file contents look the same. Can't we add a new test line or something to the existing file?

quantum marked 4 inline comments as done.Jul 16 2019, 1:58 PM
quantum added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
191 ↗(On Diff #210137)

Yes, it's right. Good point regarding the message, I'll fix it.

llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll
1 ↗(On Diff #210137)

No, the difference is that in this file, we must say

@tls = internal thread_local global i32 0

whereas in the other file, it has to say

@tls = internal thread_local(localexec) global i32 0

I thought about using grep, but @tlively suggested otherwise.

quantum updated this revision to Diff 210169.Jul 16 2019, 2:02 PM
quantum marked 2 inline comments as done.

Use better error message.

quantum updated this revision to Diff 210192.Jul 16 2019, 3:03 PM

Rebase onto latest LLVM.

aheejin accepted this revision.Jul 16 2019, 3:20 PM
This revision is now accepted and ready to land.Jul 16 2019, 3:20 PM
This revision was automatically updated to reflect the committed changes.