sunfish added inline comments.
================ Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1279 + if (const GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(Op)) + return WebAssembly::isManagedAddressSpace(GA->getAddressSpace()); + ---------------- sbc100 wrote: > sunfish wrote: > > tlively wrote: > > > tlively wrote: > > > > Actually, should we enforce that these LLVM IR globals be thread_local, > > > > since the resulting Wasm globals will be thread_local? I don't know if > > > > that will affect any optimizations, but it seems like a more accurate > > > > modeling. If we do that, > > > > `CoalesceLocalsAndStripAtomics::stripThreadLocals` in > > > > WebAssemblyTargetMachine.cpp will have to be updated to not strip the > > > > thread locals corresponding to Wasm globals. > > > I'd be fine handling this in a follow-up if you want to get this landed. > > Note that this will be true of Worker-based threads, but not of the > > expected future "wasm native threads". I wonder if this means that > > clang/llvm will (eventually) need to be aware of this difference. > Don't you think is very likely that even in "wasm native threads" we would > want to opt into sharing like we do with wasm memories? That path would > seem better for compatibility with existing worker-based threading. > > Obviously once we do have shared wasm globals we will need to modify llvm's > assumptions, but for now I think it is a reasonable assumption/assertion that > wasm globals are TLS. Yeah, I agree we'll likely want some way to opt into sharing. So the difference here is, worker-based threads won't have the option of sharing, so maybe requiring `thread_local` makes sense. But native threads could opt into sharing, so they could be `thread_local` or not. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101608/new/ https://reviews.llvm.org/D101608 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
