quantum marked 20 inline comments as done. quantum added a comment. I'll clean up the table gen stuff.
================ Comment at: clang/include/clang/Basic/BuiltinsWebAssembly.def:33 +// Thread-local storage +TARGET_BUILTIN(__builtin_wasm_tls_size, "z", "nc", "bulk-memory") + ---------------- 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. ================ Comment at: lld/test/wasm/tls.ll:57 +; memory.init 0, 0 +; end + ---------------- aheejin wrote: > Hmm, I think there might be a way to actually print disassembly results. > There are '*.test' files in test directory, in which we have some examples. > For example, [[ > https://github.com/llvm/llvm-project/blob/master/lld/test/wasm/export-table.test > | this test ]] has a sequence of commands you want to run, and you can put > input files in a separate directory. That test uses `obj2yaml`, but can we > possibly use `llvm-objdump` or something to disassemble? We already know that we can do something like Run: obj2yaml %t.wasm | sed -n '/Body:/{s/^\s*Body:\s*//;s/../0x& /gp}' | llvm-mc -disassemble -triple=wasm32 to compare the actual assembly. As for `llvm-objdump`, it seems to be unable to disassemble the WASM properly: ``` .../tools/lld/test/wasm/Output/tls.ll.tmp.wasm: file format WASM Disassembly of section CODE: 00000000 CODE: # 4 functions in section. 1: 02 00 block invalid_type 3: 0b end 4: 10 00 call 0 6: 20 00 local.get 0 8: 24 01 global.set 1 a: 20 00 local.get 0 c: 41 00 i32.const 0 e: 41 08 i32.const 8 10: fc 08 00 00 memory.init 0, 0 14: 0b end 15: 0f return 16: 00 llvm-objdump: lib/Target/WebAssembly/WebAssemblyGenAsmWriter.inc:2032: void llvm::WebAssemblyInstPrinter::printInstruction(const llvm::MCInst *, llvm::raw_ostream &): Assertion `Bits != 0 && "Cannot print this instruction."' failed. ``` ================ Comment at: lld/wasm/Driver.cpp:543 + "__wasm_init_tls", WASM_SYMBOL_VISIBILITY_HIDDEN, + make<SyntheticFunction>(I32ArgSignature, "__wasm_init_tls")); + ---------------- aheejin wrote: > Does this TLS thing work when `Config->Shared == true`, i.e., do we create > TLS globals even when this is a library? Since TLS is module specific (we can't allocate memory for other modules), we must in fact generate this symbol for every module. Shared library support will not be implemented in this diff, however. ================ Comment at: lld/wasm/Writer.cpp:431 + error("'bulk-memory' feature must be used in order to use thread-local " + "storage"); + ---------------- aheejin wrote: > Should we check for "mutable-global" feature too? Do we need to? I thought it's always available since we use it for the stack pointer. ================ Comment at: lld/wasm/Writer.cpp:514 if (G->getGlobalType()->Mutable) { // Only the __stack_pointer should ever be create as mutable. + assert(G == WasmSym::StackPointer || G == WasmSym::TLSBase); ---------------- aheejin wrote: > Now `__tls_base` is also mutable, I think we should fix the comment Will do. ================ Comment at: lld/wasm/Writer.cpp:769 + std::string BodyContent; + { + raw_string_ostream OS(BodyContent); ---------------- aheejin wrote: > Any reason for the new block scope? Yes, the `raw_string_ostream` must destruct by the end of the scope. This is the pattern used in other functions above. ================ Comment at: lld/wasm/Writer.cpp:777 + break; + } + ---------------- aheejin wrote: > Is it guaranteed that there's only one TLS segment? Yes, all the TLS input segments will be merged into `.tdata`. ================ Comment at: llvm/include/llvm/IR/IntrinsicsWebAssembly.td:134 + [], + [IntrNoMem, IntrSpeculatable]>; + ---------------- 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. ================ Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1095 +WebAssemblyTargetLowering::LowerGlobalTLSAddress(SDValue Op, + SelectionDAG &DAG) const { + SDLoc DL(Op); ---------------- aheejin wrote: > If you do the conversion in `WebAssemblyISelDAGToDAG.cpp`, you don't need to > create `WebAssemblyISD` node, `SDTypeProfile`, and `SDNode` for every single > instruction you want to generate. This `WebAssemblyISelLowering.cpp` is a > part of legalization so it cannot generate machine instructions directly, > whereas `WebAssemblyISelDAGToDAG.cpp` is a part of instruction selection so > you have direct access to machine instructions. I think moving routines there > can be cleaner? It seems most other architectures do it in `*TargetLowering`, so I decided to do the same. ================ Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1109 + WebAssemblyISD::CONST_I32, DL, VT, + DAG.getTargetGlobalAddress(GA->getGlobal(), DL, VT, GA->getOffset(), 0)); + ---------------- aheejin wrote: > Does this offset work when there are non-thread-local globals too? Yes, the linker handles the offsets. All thread locals are grouped together by the linker. ================ Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:192 Stripped |= stripAtomics(M); - Stripped |= stripThreadLocals(M); } ---------------- aheejin wrote: > Looks like this feature requires both bulk memory and mutable global. > Shouldn't we strip thread locals if either is not enabled? Do we want to do that? I think having your thread locals vanish if you didn't pass `-mbulk-memory` is surprising. It might make more sense to strip thread locals if `-pthread` is not passed. ================ Comment at: llvm/test/CodeGen/WebAssembly/target-features-tls.ll:23 ; ATOMICS-NEXT: .ascii "atomics" ; ATOMICS-NEXT: .tbss.foo,"",@ ---------------- aheejin wrote: > Is thread local feature still dependent on atomics? If not, do we still need > to test this with both atomics on and off? This now looks rather dependent on > bulk memory and mutable global. Should we test this feature with those > options instead? (I can be mistaken, so please let me know if so) On second thought, this test is somewhat meaningless now, since the thread local is always created regardless of whether atomics are enabled. What does @tlively think? ================ Comment at: llvm/test/CodeGen/WebAssembly/tls.ll:16 + ; O2-NEXT: i32.const tls + ; O2-NEXT: i32.add + ; O2-NEXT: return ---------------- aheejin wrote: > If the only difference of O0 and O2 results is the order of `global.get` and > `i32.const`, you can use [[ > https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive | > `CHECK-DAG` ]] directive, like > ``` > ; CHECK-DAG: global.get __tls_base > ; CHECK-DAG: i32.const tls > ... > ``` > > The same for other functions too. Maybe we don't need separate O0 and O2 > testing in this file. Will do. I didn't know `CHECK-DAG` existed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
