[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR
Dan Gohman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu May 6 10:43:50 PDT 2021
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
----------------
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.
================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1279
+ if (const GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(Op))
+ return WebAssembly::isManagedAddressSpace(GA->getAddressSpace());
+
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101608/new/
https://reviews.llvm.org/D101608
More information about the cfe-commits
mailing list