[PATCH] D101913: [CodeGen][WebAssembly] Better lowering for WASM_SYMBOL_TYPE_GLOBAL symbols

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:179
+
+  assert(!GV->isThreadLocal());
+
----------------
wingo wrote:
> sbc100 wrote:
> > Why this?   As it happens I think that globals are *all* thread local by default (at least we assume they are in the current threading ABI in llvm and emscripten).   We don't have a way to share global between threads to (like we do with memory).
> Good point; I hadn't thought about this.  For context I was looking at what the base "getGlobalVariable" does and where it has conditional blocks for non-default flags, and asserting that no one had made non-default settings on the symbol.
> 
> But... semantically what's the story here?  I guess that although you can share globals between instances, you can't post them to a web worker, so they are always thread-local.
> 
> Should WASM_SYMBOL_TYPE_GLOBAL symbols be marked by default as being thread-local then, perhaps?

> Should WASM_SYMBOL_TYPE_GLOBAL symbols be marked by default as being thread-local then, perhaps?


I guess so .. at least if we are talking about the current threading support on the web.  In the future globals will most likely be share-able.. or on other targets they might even be shared by default.  But given that llvm's thread support is currently only used by the web targets it would make sense to me yes.   Does't have to be part of this change though.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:204
+    OutStreamer->emitLabel(Sym);
+    OutStreamer->AddBlankLine();
+  }
----------------
wingo wrote:
> sbc100 wrote:
> > There is no actual initializer here.. it will always be zero I guess?  Do we need a TODO?
> Added the TODO.  This one is tricky because there's no MC support for global initializers yet, AFAIU.
> 
> Not sure how it should work, tbh; though the binary format specifies that this should be a sequence of constant instructions followed by `end`, I think I would want to restrict this one from the MC side to be some form of MCConstantExpr, serialized either as part of the `.globaltype` line or as a new `.globalvalue SYM, EXPR` form.  WDYT?  Would be a followup I guess.

> Not sure how it should work, tbh; though the binary format specifies that this should be a sequence of constant instructions followed by `end`, I think I would want to restrict this one from the MC side to be some form of MCConstantExpr, serialized either as part of the `.globaltype` line or as a new `.globalvalue SYM, EXPR` form.  WDYT?  Would be a followup I guess.

Yeah, I'm not sure about this either.   Using actual instructions seems overkill, but at the same time matches the spec.   I think it would be great if we could make it work... but also a more declarative mode might sense given the severe restrictions on instructions in cont expressions today.  

How close are you to needing initializer values here?  Can we punt for a little longer? :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101913/new/

https://reviews.llvm.org/D101913