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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

lgtm with comments/questions.



================
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;
----------------
Is Flags and enum.. in which case should we name the number values and should these comments say flags & 2?


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:673
+      return expect(AsmToken::EndOfStatement, "EOL");
+    }
+
----------------
tlively wrote:
> @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?
There are still issues with roundtriping in its full generality.   Open any bug you find and we can incrementally improve things.


================
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.
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57938