[PATCH] D130053: [WebAssembly] Use `localexec` as default TLS model for non-Emscripten targets

[Andrew Brown via Phabricator via llvm-commits]

abrown added inline comments.


================
Comment at: llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll:2
-; RUN: not llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics 2>&1 | FileCheck %s --check-prefix=ERROR
-; RUN: not llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics -fast-isel 2>&1 | FileCheck %s --check-prefix=ERROR
 ; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -mattr=+bulk-memory,atomics --mtriple wasm32-unknown-emscripten | FileCheck %s --check-prefixes=CHECK,TLS
----------------
sunfish wrote:
> penzn wrote:
> > sunfish wrote:
> > > abrown wrote:
> > > > @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.
> > > 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.
> > The test would fail if these two lines are preserved, because they check that default behavior is to throw an error.
> 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.
> 
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.


================
Comment at: llvm/test/CodeGen/WebAssembly/tls-local-exec.ll:94
 
- at tls_external = external thread_local(localexec) global i32, align 4
+ at tls_external = external thread_local global i32, align 4
 
----------------
sbc100 wrote:
> 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?
Sure, that makes sense. Can you point me to an example of how to setup such a macro?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130053/new/

https://reviews.llvm.org/D130053