[PATCH] D121349: [WebAssembly] Second phase of implementing extended const proposal.

Thu Mar 10 17:32:48 PST 2022

dschuff added inline comments.

================
Comment at: llvm/lib/Object/WasmObjectFile.cpp:211
+      case wasm::WASM_OPCODE_I32_CONST:
+        readVarint32(Ctx);
+        break;
----------------
This loop could at least be simplified if we just used `readULEB128` instead of all these typed versions e.g. `readVarint32` since we aren't using the value and maybe don't care if the encoded value overflows.

================
Comment at: llvm/lib/Object/WasmObjectFile.cpp:200
+  if (!Expr.Extended) {
+    uint8_t EndOpcode = readOpcode(Ctx);
+    if (EndOpcode != wasm::WASM_OPCODE_END)
----------------
sbc100 wrote:
> dschuff wrote:
> > this is a bit confusing. If there's just a const/global.get/ref.null followed by an end, it's MVP, otherwise it's extended. But can there be 2 consts in a row? Seems like that would also be accepted here? and then line 208 would read a 3rd opcode?
> Line 206 resets the the read pointer back to the start of the init expr.
> 
> Its kind of sad we have to actually parse of the initexpr here.  Turns out that because the initexpr doesn't encode its own length (unlike a function) it cannot be skipped over which means that is not possible to write a file decoder that doesn't also have an instruction decodeer :(
I guess you can skip over the whole section, but not each individual init expr. Is there any use case for wanting to skip over or avoid parsing some subset of initexprs? Even our file decoders in LLVM still parsed the const exprs of MVP.
... I guess this file itself is an example of where we want to know the location/size of the initexpr but don't care about evaluating it. 
I wonder if it would be worth adding an extra byte to every initexpr to encode the size. Seems like a lot when each expr is still just a few bytes.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121349