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.
Details
Diff Detail
- Repository
- rLLD LLVM Linker
Event Timeline
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? |
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? |
- 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.
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. |
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp | ||
---|---|---|
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) |
290 ↗ | (On Diff #186061) | Call expect here instead? |
- Move .init_flags parsing to WasmAsmParser.cpp
- Fix old tests to use thread-model=single
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. |
llvm/include/llvm/BinaryFormat/Wasm.h | ||
---|---|---|
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. |
134 ↗ | (On Diff #186377) | InitFlags? |
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. |
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?
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. |
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? |
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. |
llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h | ||
---|---|---|
20 ↗ | (On Diff #186385) | This was no longer necessary at all, so I removed it. |