[PATCH] D64537: [WebAssembly] Implement thread-local storage for non-PIC cases

[Guanzhong Chen via Phabricator via llvm-commits]

quantum marked 5 inline comments as done.
quantum added inline comments.


================
Comment at: clang/include/clang/Basic/BuiltinsWebAssembly.def:33
+// Thread-local storage
+TARGET_BUILTIN(__builtin_wasm_tls_size, "z", "nc", "bulk-memory")
+
----------------
quantum wrote:
> aheejin wrote:
> > Why is it `c`(const)? According to [[ https://github.com/llvm/llvm-project/blob/e6695821e592f95bffe1340b28be7bcfcce04284/clang/include/clang/Basic/Builtins.h#L104-L108 | this comment ]], this is true if this function has no side effects and doesn't read memory, i.e., the result should be only dependent on its arguments. Can't wasm globals be memory locations in machines?
> I was thinking that since the global is immutable, so the value is always a constant.
According to @tlively, there is no solid definition on whether a global (especially a constant one), counts as memory access. For now, I am going to change this to not constant. We can always change it back later.


================
Comment at: llvm/include/llvm/IR/IntrinsicsWebAssembly.td:134
+            [],
+            [IntrNoMem, IntrSpeculatable]>;
+
----------------
quantum wrote:
> aheejin wrote:
> > - Why is it speculatable?
> > - I'm not sure if it's `InstNoMem`, because wasm globals can be memory locations after all. What do other people think?
> @tlively suggested that I do this. It should be speculatable because it has no side effects (since it returns a constant). As for `InstrNoMem`, I am not sure if globals are memory, and whether reading a constant counts as memory access.
I think I am going to remove these attributes for now. We can add them back if we end up deciding that immutable globals are not memory accesses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64537