sunfish added inline comments.
================ Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:39 + // pointers are lowered to global.get / global.set or local.get / local.set, + // as appropriate. + WASM_ADDRESS_SPACE_MANAGED = 1 ---------------- tlively wrote: > sunfish wrote: > > Sorry to throw more paint at the bikeshed here, but as someone who's only > > following along at a high-level here, I found it confusing whether this is > > talking about the wasm globals themselves, or the objects referred to by > > reference values in the wasm globals. I think the feature here is talking > > about the wasm globals themselves, but "managed" initially made me think it > > might be talking about the objects they reference, which in a browser > > context especially are "managed" in every sense of the word. > Fair point. What does everyone think about `INDEXED`, because it is used to > represent objects given static indexes (in the WebAssembly sense) in the > final binary. Do I understand correctly that global variables and local variables are being assigned addresses within the same conceptual address space here? How about `WASM_VARIABLES` or `WASM_VARS`? The wasm spec terms for these are global variables and local variables. ================ 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: > > 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. > > > Yup. And who know .. maybe by the time shared globals and native threads are > added, Workers might also add shared globals? It's possible :-). In any case, I don't think it's anything we need to do anything about right now. 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
