sbc100 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());
+
----------------
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.


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

Reply via email to