This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Use `localexec` as default TLS model for non-Emscripten targets
ClosedPublic

Authored by abrown on Jul 18 2022, 4:41 PM.

Details

Summary

Only Emscripten supports dynamic linking with threads. To use thread-local
storage for other targets, this change defaults to the localexec model.

Diff Detail

Event Timeline

abrown created this revision.Jul 18 2022, 4:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2022, 4:41 PM
abrown published this revision for review.Jul 18 2022, 4:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2022, 4:47 PM
sbc100 accepted this revision.Jul 18 2022, 5:15 PM

lgtm! Although we probably need to test.. perhaps there already is one which needs updating due to this test? I imagine it would be in the test/CodeGen/WebAssembly directory.

This revision is now accepted and ready to land.Jul 18 2022, 5:15 PM

LGTM too. For testing, I think it would work to make a copy of llvm/test/CodeGen/WebAssembly/tls-local-exec.ll and change its thread_local(localexec)s to thread_locals.

penzn added a subscriber: penzn.Jul 19 2022, 10:56 AM
abrown updated this revision to Diff 445880.Jul 19 2022, 11:03 AM
This comment was removed by abrown.
abrown updated this revision to Diff 445881.Jul 19 2022, 11:04 AM
This comment was removed by abrown.
abrown updated this revision to Diff 445883.Jul 19 2022, 11:06 AM
This comment was removed by abrown.
abrown updated this revision to Diff 445885.Jul 19 2022, 11:09 AM
This comment was removed by abrown.
abrown updated this revision to Diff 445889.Jul 19 2022, 11:13 AM

[WebAssembly] Use localexec as default TLS model for non-Emscripten targets

Only Emscripten supports dynamic linking with threads. To use thread-local
storage for other targets, this change defaults to the localexec model.

Additionally, this changes the tests slightly:

  • Ignore non-Emscripten test cases for general dynamic TLS model
  • Use default thread-local model (i.e., localexec)
llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll
2 ↗(On Diff #445885)

@sbc100, @sunfish: I am not exactly sure what to do here. Ideally it would be nice to test that the "no-Emscripten, general dynamic, no DSO local case" actually emits the same code as localexec, but this test does not seem set up for this.

sunfish added inline comments.Jul 19 2022, 2:32 PM
llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll
2 ↗(On Diff #445885)

I suggest leaving this test as-is, because updating it to handle non-Emscripten targets would require adding all new CHECK lines, which would be a lot of clutter. I suggest just adding RUN lines to tls-initial-exec.ll, or perhaps making a copy of that file, to test this feature.

penzn added inline comments.Jul 19 2022, 2:41 PM
llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll
2 ↗(On Diff #445885)

The test would fail if these two lines are preserved, because they check that default behavior is to throw an error.

sunfish added inline comments.Jul 20 2022, 10:05 AM
llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll
2 ↗(On Diff #445885)

Ah, right. Can we just disable non-Emscripten targets here? My main thought here is to avoid needing to add all the CHECKs for local-exec code in tls-general-dynamic.ll.

sbc100 added inline comments.Jul 21 2022, 8:25 AM
llvm/test/CodeGen/WebAssembly/tls-local-exec.ll
94

Shouldn't we leave this file as is, with the explicit TLS model? (according the language reference the default thread model is general dynamic: https://llvm.org/docs/LangRef.html#thread-local-storage-models "If no explicit model is given, the “general dynamic” model is used.").

I guess you are trying to test here that general dynamic is treated "as if" its local-exec, but we also want to test how the explicit local-exec works, no? Would we use a macro to and run this entire test with (localexec) and with the empty string to check they produce the same results?

abrown added inline comments.Jul 21 2022, 1:28 PM
llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll
2 ↗(On Diff #445885)

Yeah, that's what the two lines removed at the top do here. (You'll notice line 5 doesn't target Emscripten but that is just doing a check when bulk memory is disabled, -bulk-memory). I agree that adding a bunch to this file doesn't make much sense. Perhaps I can add a one-off file that targets non-Emscripten and the non-local DSO case and shows how actually in that case we are overriding the codegen to look like localexec. What do you think of something like that, e.g., tls-general-dynamic-override.ll?

Another option is to modify the file as I have here and that's it--nothing extra. I guess the "override with localexec" isn't getting tested specifically then, but it seems like tls-local-exec.ll is covering some of this.

llvm/test/CodeGen/WebAssembly/tls-local-exec.ll
94

Sure, that makes sense. Can you point me to an example of how to setup such a macro?

sbc100 added inline comments.Jul 21 2022, 2:54 PM
llvm/test/CodeGen/WebAssembly/tls-local-exec.ll
94

You can do git grep "FileCheck.* -D" to find a lot of examples in llvm.

abrown updated this revision to Diff 446963.Jul 22 2022, 1:27 PM

Update to fix tests, clang-format

This change used git clang-format origin/main -- llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp to properly format that code and adds some sed substitution in tls-local-exec.ll to check both the "specify localexec explicitly" and "emit localexec even under the generaldynamic default" cases. I believe this should also cover the cases removed from tls-general-dynamic.ll.

sbc100 added inline comments.Jul 22 2022, 1:40 PM
llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll
6 ↗(On Diff #446963)

Should we change the triple here to be wasm32-unknown-emscripten and remove it from the lines above.

Also, might be worth adding a comment above. Something like:

"This is a test for the experimental emscripten-specific general dynamic TLS support. See tls-local-exec.ll for non-emscripten targets (since they lower all TLS to localexec)"

llvm/test/CodeGen/WebAssembly/tls-local-exec.ll
7

Maybe add something like "...on non-emscripten targets which don't currently support dynamic linking."

Also, should we run this with explicit general dynamic too?

abrown updated this revision to Diff 446972.Jul 22 2022, 1:54 PM

Update tests with @sbc100's suggestions

sbc100 accepted this revision.Jul 22 2022, 2:07 PM
sbc100 added inline comments.
llvm/test/CodeGen/WebAssembly/tls-local-exec.ll
7

How about explicit general dynamic TLS mode here too (since I think that is what clang generates)?

abrown added inline comments.Jul 22 2022, 2:13 PM
llvm/test/CodeGen/WebAssembly/tls-local-exec.ll
7

Yeah, here's what I get when I inject (generaldynamic):

/.../llvm-project/build/bin/llc: error: /.../llvm-project/build/bin/llc: <stdin>:103:30: error: expected localdynamic, initialexec or localexec
@tls = internal thread_local(generaldynamic) global i32 0
penzn added a comment.Jul 25 2022, 1:09 AM
This comment was removed by penzn.
llvm/test/CodeGen/WebAssembly/tls-local-exec.ll
94

Shouldn't we leave this file as is, with the explicit TLS model? (according the language reference the default thread model is general dynamic: https://llvm.org/docs/LangRef.html#thread-local-storage-models "If no explicit model is given, the “general dynamic” model is used.").

I guess you are trying to test here that general dynamic is treated "as if" its local-exec, but we also want to test how the explicit local-exec works, no? Would we use a macro to and run this entire test with (localexec) and with the empty string to check they produce the same results?

94

Sure, that makes sense. Can you point me to an example of how to setup such a macro?

94

Shouldn't we leave this file as is, with the explicit TLS model? (according the language reference the default thread model is general dynamic: https://llvm.org/docs/LangRef.html#thread-local-storage-models "If no explicit model is given, the “general dynamic” model is used.").

Target is allowed to change model even if it is supported, "The target may choose a different TLS model if the specified model is not supported, or if a better choice of model can be made."

I guess you are trying to test here that general dynamic is treated "as if" its local-exec, but we also want to test how the explicit local-exec works, no? Would we use a macro to and run this entire test with (localexec) and with the empty string to check they produce the same results?

This might affect more than general-dynamic vs local-exec, but I am not sure it is realistically to test for all possible ones. I think macro would be on the IR side, not FileCheck side (latter should get the same input), and I can't recall how we express something like that.

penzn accepted this revision.Jul 25 2022, 1:21 AM

Oops, commented on outdated version (also had some issues with UI leaving repeated inline comments, sorry).

Out of curiosity, why disabling bulk memory disables TLS? Couldn't find why that is the case in tools conventions.

llvm/test/CodeGen/WebAssembly/tls-local-exec.ll
7

I don't think it is allowed to be specified directly, it is default-only mode.

Out of curiosity, why disabling bulk memory disables TLS? Couldn't find why that is the case in tools conventions.

Because TLS data is stored a "passive" segment which has to be loaded using the memory.init instruction. (Both passive segments and this instruction are part of the bulk-memory proposal).

Here is the memory.init instruction being output by the linker: https://github.com/llvm/llvm-project/blob/07aa8fc8db6b4b8581e0ba8ef4a66274023c0b59/lld/wasm/Writer.cpp#L1439

penzn added a comment.Jul 25 2022, 1:06 PM

Because TLS data is stored a "passive" segment which has to be loaded using the memory.init instruction. (Both passive segments and this instruction are part of the bulk-memory proposal).

Thanks! Should something about this added to tool conventions? I understand why may be it wasn't - this is internal to how compiler can implement TLS, not "handshake" between tools.

Because TLS data is stored a "passive" segment which has to be loaded using the memory.init instruction. (Both passive segments and this instruction are part of the bulk-memory proposal).

Thanks! Should something about this added to tool conventions? I understand why may be it wasn't - this is internal to how compiler can implement TLS, not "handshake" between tools.

I think this should be tooling-conventions somewhere yes.. I don't think we have anything there yet about TLS or threading.

This revision was landed with ongoing or failed builds.Jul 25 2022, 1:26 PM
This revision was automatically updated to reflect the committed changes.