Page MenuHomePhabricator

[WebAssembly] Update MC for bulk memory
ClosedPublic

Authored by tlively on Feb 7 2019, 9:35 PM.

Details

Summary

Rename MemoryIndex to InitFlags and implement logic for determining
data segment layout in ObjectYAML and MC. Also adds a "passive" flag
for the .section assembler directive although this cannot be assembled
yet because the assembler does not support data sections.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

tlively created this revision.Feb 7 2019, 9:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 9:35 PM
tlively marked an inline comment as done.Feb 8 2019, 11:21 AM
tlively added inline comments.
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
673 ↗(On Diff #185910)

@aardappel, I wanted to test that this directive would properly round trip through the assembler and disassembler, but I wasn't able to get the assembler to handle a data section properly. Is this something that you would expect to work at the moment?

sbc100 added a comment.Feb 8 2019, 1:47 PM

lgtm with comments/questions.

llvm/include/llvm/BinaryFormat/Wasm.h
136 ↗(On Diff #185910)

Is Flags and enum.. in which case should we name the number values and should these comments say flags & 2?

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
673 ↗(On Diff #185910)

There are still issues with roundtriping in its full generality. Open any bug you find and we can incrementally improve things.

llvm/test/MC/WebAssembly/init-flags.ll
5 ↗(On Diff #185910)

What is .cond_init? I don't see any other mention of it?

Also, am I to understand that enabling bulk memory causes all the data sections to be passive by default?

tlively updated this revision to Diff 186061.Feb 8 2019, 3:42 PM
  • Switch to using ThreadModel instead of hasBulkMemory to determine segment initializer types. This creates a dependency on D57874, which makes "single" the default thread model for wasm.
tlively marked 2 inline comments as done.Feb 8 2019, 3:54 PM
tlively added inline comments.
llvm/include/llvm/BinaryFormat/Wasm.h
136 ↗(On Diff #185910)

No, in the bulk memory proposal, the meaningful values are referred to only as 1, 2, and 3. Perhaps it would make more sense to name this field "InitType" and the directive ".init_type" or similar and create a corresponding enum for the different values it can take on. I worry that there are already too many too many things called "types" floating around, though. What do you think?

llvm/test/MC/WebAssembly/init-flags.ll
5 ↗(On Diff #185910)

Good catch, this should .init_flags. .cond_init was an older name I had for the directive. In the original diff yes, enabling bulk memory was meant to cause all data sections to be passive. Now it is setting -thread-model=posix (or anything non-single threaded, if any other options existed) that controls this.

aardappel added inline comments.Feb 11 2019, 11:24 AM
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
290 ↗(On Diff #186061)

Call expect here instead?

673 ↗(On Diff #185910)

Nope, was actually just going to look into data sections.

673 ↗(On Diff #185910)

Also, parsing of segments is in WasmAsmParser.cpp, so this code may make more sense there (if its about the container format, not the instruction set)

tlively updated this revision to Diff 186376.Feb 11 2019, 5:28 PM
tlively marked an inline comment as done.
  • Move .init_flags parsing to WasmAsmParser.cpp
  • Fix old tests to use thread-model=single
tlively updated this revision to Diff 186377.Feb 11 2019, 5:34 PM
  • Remove round trip from init-flags.ll because both directions were already tested.
tlively updated this revision to Diff 186385.Feb 11 2019, 6:34 PM
tlively marked 2 inline comments as done.
  • Use enum for flags instead of int literals
tlively added inline comments.Feb 11 2019, 6:34 PM
llvm/include/llvm/BinaryFormat/Wasm.h
136 ↗(On Diff #185910)

I realized that there was a clean way to interpret the values as flags, so I went ahead and made that change.

sbc100 added inline comments.Feb 12 2019, 4:06 PM
llvm/include/llvm/BinaryFormat/Wasm.h
134 ↗(On Diff #186377)

InitFlags?

136 ↗(On Diff #185910)

I'm happy with ".init_flags" and the field being called InitFlag, but I think it would be better to have the the 3 possible values to be in an enum or #define in BinaryFormat/Wasm.h rather than magic numbers in the sources.

llvm/include/llvm/ObjectYAML/WasmYAML.h
115 ↗(On Diff #186377)

InitFlags?

llvm/test/MC/WebAssembly/external-data.ll
1 ↗(On Diff #186385)

Why do this, given that single is still the default?

Sorry my last comments were out-of-date (I think I wrote them yesterday) and don't all apply anymore :)

Without the context of MC here there's no indication what kind of flags the .init_flags directive refers to. Maybe call it init_segment_flags?

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
20 ↗(On Diff #186385)

You don't need to include this here, you can just forward declare class MCSectionWasm down on line26 with the others.

Without the context of MC here there's no indication what kind of flags the .init_flags directive refers to. Maybe call it init_segment_flags?

Better still perhaps we can attach it to the segment/section declaring in the the asm itself? It should be noted that wouter is working changing this right now.. so it probably OK to land this and then consolidate/iterate?

tlively planned changes to this revision.Feb 13 2019, 1:12 PM
tlively marked an inline comment as done.

The way this CL is written now, it parses .init_flags in WasmAsmParser and writes the new data segment headers in WasmObjectWriter, but the streaming is handled in WebAssemblyTargetStreamer. This mismatch between container-level and target-level processing is no good, so I'm going to refactor the CL to use MCWasmStreamer. Unfortunately that means I will have to add new methods to the top level MCStreamer class. There are plenty of other container-specific methods in MCStreamer already, although not so much for wobjects yet. While I'm at it I will change .init_flags so that instead of applying to the current section (which must be a data section), it will take the name of the data section as an argument.

But before I do all that, I'm going to experiment with using the flags mechanism of the .section directive that is used by other container formats for similar purposes. This would mean no new directives and no polluting shared interfaces. Yay!

llvm/test/MC/WebAssembly/external-data.ll
1 ↗(On Diff #186385)

Single is the default for clang, but not for llc.

sbc100 added inline comments.Feb 13 2019, 2:16 PM
llvm/test/MC/WebAssembly/external-data.ll
1 ↗(On Diff #186385)

Hmm, that doesn't seem great. We don't want llc, but default, generating code that requires the threads proposal. Maybe we should fix that?
(unless its particularly awkward).

tlively edited the summary of this revision. (Show Details)Feb 14 2019, 10:46 AM
tlively updated this revision to Diff 186871.Feb 14 2019, 10:47 AM
tlively marked an inline comment as done.
  • Replace .init_flags directive with a .section directive flag, "passive"
tlively marked 2 inline comments as done.Feb 14 2019, 10:51 AM
tlively added inline comments.
llvm/test/MC/WebAssembly/external-data.ll
1 ↗(On Diff #186385)

I'm not sure that needs to be changed, but if it does we should do that as a separate CL.

tlively updated this revision to Diff 186878.Feb 14 2019, 11:08 AM
tlively marked 2 inline comments as done.
  • Remove unnecessary #include
tlively added inline comments.Feb 15 2019, 4:02 PM
llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
20 ↗(On Diff #186385)

This was no longer necessary at all, so I removed it.

tlively updated this revision to Diff 187106.Feb 15 2019, 4:08 PM
  • Remove unnecessary #include
This comment was removed by asl.
asl added a subscriber: asl.
sbc100 accepted this revision.Feb 19 2019, 1:56 PM
This revision is now accepted and ready to land.Feb 19 2019, 1:56 PM
This revision was automatically updated to reflect the committed changes.