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

[Derek Schuff via Phabricator via llvm-commits]

dschuff accepted this revision.
dschuff added a comment.
This revision is now accepted and ready to land.

This code is fine as a reflection of the current state of the proposal though.



================
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:
> > 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.
> I actually already opened a bug to discuss the possibility of adding a size so we could avoid writing and instruction decoder here: https://github.com/WebAssembly/design/issues/1443.
> 
> I'm guessing it would be tricky to add at this point.
It's certainly not too late to change the extended-const proposal. We can follow up there.


================
Comment at: llvm/test/ObjectYAML/wasm/extended_const_expressions.yaml:19
+        Mutable:     false
+        InitExpr:
+          # (global.get[0x23] 0x1 i32.const[0x41] 0x2 i32.add[0x6a] end[0x0b])
----------------
Maybe it's worth checking an invalid constexpr (to test that we either do or don't care about validity in this particular code)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121349