[PATCH] D58315: [WebAssembly] Add .shared directive for shared memory

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 3 inline comments as done.
tlively added inline comments.


================
Comment at: llvm/include/llvm/MC/MCSymbolWasm.h:26
   Optional<wasm::WasmEventType> EventType;
+  uint8_t MemFlags = 0;
 
----------------
sbc100 wrote:
> Seems strange to attach this to a symol.
Different kinds of symbols already store their special information in this object, such as their Signatures or Types. I think it makes sense for memory imports to do the same.


================
Comment at: llvm/test/MC/WebAssembly/assembler-binary.ll:25
+; ASM:          .text
+; ASM:          .shared
+; ASM-NEXT:     .file "assembler-binary.ll"
----------------
sbc100 wrote:
> This looks like its starting a new section.   Could we call this ".shared_memory"?  Or is there some other place in the asm syntax there this attribute might be specified?
> 
> Actually do we need this to be in the .s format at all?  seems like it could be specified as an assembler flag?  Like the march/mcpu stuff.. that doesn't live in the .s format right?
Yes, calling it ".shared_memory" would be fine, although it is really meant to communicate that the entire module was produced to be thread-aware, not just that the memory is shared. That this corresponds to the shared bit being set in the object file is mostly an implementation detail. I'm open to suggestions for other places this information might live, but up at the top of the file seemed appropriate because it applies to the entire module.

I believe the march/mcpu options are not recorded in the .s format because they only affect codegen, which has already taken place by the time you have an assembly file. In contrast, for webassembly the information that code was produced by a thread-aware toolchain needs to be propagated through to the linker alongside that code, whether or not there is a separate assembly step in between. There is no way to propagate this information through an intermediate assembly step except by assembler directive.


================
Comment at: llvm/test/MC/WebAssembly/event-section.ll:1
-; RUN: llc -filetype=obj -exception-model=wasm -mattr=+exception-handling %s -o - | obj2yaml | FileCheck %s
-; RUN: llc -filetype=obj -exception-model=wasm -mattr=+exception-handling %s -o - | llvm-readobj -s | FileCheck -check-prefix=SEC %s
+; RUN: llc -filetype=obj -thread-model=single -exception-model=wasm -mattr=+exception-handling %s -o - | obj2yaml | FileCheck %s
+; RUN: llc -filetype=obj -thread-model=single -exception-model=wasm -mattr=+exception-handling %s -o - | llvm-readobj -s | FileCheck -check-prefix=SEC %s
----------------
sbc100 wrote:
> Can we make this the default instead?
I'm not sure we can. The option and its default value are are statically defined in include/llvm/CodeGen/CommandFlags.inc, and I don't think it's possible to make the default value of an option like that depend on the value of another option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58315