[PATCH] D57938: [WebAssembly] Update MC for bulk memory

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/Wasm.h:136
+  uint32_t MemoryIndex; // present if flags == 2
+  WasmInitExpr Offset; // if flags != 1
   ArrayRef<uint8_t> Content;
----------------
tlively wrote:
> sbc100 wrote:
> > Is Flags and enum.. in which case should we name the number values and should these comments say flags & 2?
> 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?
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.


================
Comment at: llvm/include/llvm/BinaryFormat/Wasm.h:134
 struct WasmDataSegment {
-  uint32_t MemoryIndex;
-  WasmInitExpr Offset;
+  uint32_t Flags;
+  uint32_t MemoryIndex; // present if flags == 2
----------------
InitFlags?


================
Comment at: llvm/include/llvm/ObjectYAML/WasmYAML.h:115
   uint32_t SectionOffset;
+  uint32_t Flags;
+  uint32_t MemoryIndex;
----------------
InitFlags?


================
Comment at: llvm/test/MC/WebAssembly/external-data.ll:1
-; RUN: llc -filetype=obj %s -o - | obj2yaml | FileCheck %s
+; RUN: llc -filetype=obj -thread-model=single %s -o - | obj2yaml | FileCheck %s
 
----------------
Why do this, given that single is still the default?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57938