Page MenuHomePhabricator

[WebAssembly] Add .shared_memory directive
AbandonedPublic

Authored by tlively on Feb 15 2019, 5:07 PM.

Details

Summary

For object files, emitting the .shared_memory directive sets the
shared bit on the __linear_memory import symbol, so memory flags are
also added to wasm symbols. The directive is emitted in the AsmPrinter
if the thread model is multithreaded to communicate to the linker that
this object is built for a multithreaded context. Depends on D57938.

Event Timeline

tlively created this revision.Feb 15 2019, 5:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2019, 5:07 PM
sbc100 added inline comments.Feb 15 2019, 5:15 PM
llvm/include/llvm/MC/MCSymbolWasm.h
26

Seems strange to attach this to a symol.

llvm/test/MC/WebAssembly/assembler-binary.ll
25

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?

llvm/test/MC/WebAssembly/event-section.ll
1–2

Can we make this the default instead?

tlively marked 3 inline comments as done.Feb 15 2019, 6:37 PM
tlively added inline comments.
llvm/include/llvm/MC/MCSymbolWasm.h
26

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.

llvm/test/MC/WebAssembly/assembler-binary.ll
25

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.

llvm/test/MC/WebAssembly/event-section.ll
1–2

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.

@sbc100 I'm looking at the linker to figure out how to implement shared memory-aware linking, and it looks like the linker doesn't even look in the imports section of the wobject. Why even bother importing __linear_memory in object files at all? Is it just to make the object file a spec-compliant wasm? I think I'm going to have to make the linker look at the import section for the sole purpose of fetching out this shared bit on the memory import. This isn't a problem, but it did surprise me.

@sbc100 I'm looking at the linker to figure out how to implement shared memory-aware linking, and it looks like the linker doesn't even look in the imports section of the wobject. Why even bother importing __linear_memory in object files at all? Is it just to make the object file a spec-compliant wasm? I think I'm going to have to make the linker look at the import section for the sole purpose of fetching out this shared bit on the memory import. This isn't a problem, but it did surprise me.

Yes, its purely to make the object file valid.

I'm tempted to have the linker driven by a flag for this rather then driven by the input file.

If you were going to check that kind of think I would do it when cheking the format of the object file. Perhaps is lld::wasm::createObjectFile.

aardappel added inline comments.Feb 19 2019, 9:28 AM
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
654

This applies to the most recent symbol? Seems many directives we have explicitly refer to the symbol they're changing, like .functype, .eventtype, .globaltype, so would it make sense for this to that too?

Or is there only ever one such flag and one such symbol, ever?

tlively marked an inline comment as done.Feb 19 2019, 9:47 AM
tlively added inline comments.
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
654

There is meant to only ever be one such flag for the entire module, and the only memory import is the __linear_memory symbol that this affects. Currently the ObjectFileWriter unconditionally emits __linear_memory, and this only modifies the flags it is emitted with.

sbc100 added inline comments.Feb 19 2019, 12:45 PM
llvm/lib/MC/WasmObjectWriter.cpp
756

I guess this makes sense for completeness, but an object file really doesn't make sense to have a max for its memory.

tlively marked an inline comment as done.Feb 19 2019, 1:28 PM
tlively added inline comments.
llvm/lib/MC/WasmObjectWriter.cpp
756

Yeah, unfortunately it is necessary if we want object files to validate. Also unfortunately, it is invalid to have the limits be shared but not have a max, so we have to just come up with some arbitrary max.

tlively updated this revision to Diff 187448.Feb 19 2019, 2:46 PM
  • Rename .shared to .shared_memory, add comment
tlively retitled this revision from [WebAssembly] Add .shared directive for shared memory to [WebAssembly] Add .shared_memory directive.Feb 19 2019, 2:58 PM
tlively edited the summary of this revision. (Show Details)
sbc100 added inline comments.Feb 19 2019, 3:43 PM
llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
163

It still seems wrong to attach this information to a symbol, is there not some module level state we can attach it too?

I don't think __linear_memory should be and MCSymbol at all. We don't have a symbol type for a memory. It looks like it is referenced in WasmObjectWriter.cpp but I can't see why it would be.

tlively marked an inline comment as done.Feb 20 2019, 5:54 PM

An alternative to the approach taken in this CL that explicitly records that the object is thread-aware in a new metadata section is presented here: https://github.com/WebAssembly/tool-conventions/pull/99

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
163

Changing approaches to use a target features section would make this change to MCSymbol unnecessary (although WasmObjectWriter would still treat __linear_memory as a symbol).

sbc100 added inline comments.Feb 20 2019, 6:39 PM
llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
163

Not if I can land this first :) https://reviews.llvm.org/D58487

Seriously though, is there some other place we can attach the "shared" attribute so that it can later be read by the object writer? I don't think we need to block on the target features section.. I'm still happy with the flag on the memory import, just that we set that flag based on something other than an MCSymbol? (Not sure how reasonable this request is).

tlively abandoned this revision.Feb 27 2019, 3:20 PM

It looks like we have consensus that a target features section would be a good idea, so I'm closing out this CL.