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

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 2 inline comments as done.
tlively 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;
----------------
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?


================
Comment at: llvm/test/MC/WebAssembly/init-flags.ll:5
+; Test that enabling bulk memory causes data sections to be marked
+; with .cond_init, and that round tripping through the assembler and
+; disassembler preserves that information.
----------------
sbc100 wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57938