[PATCH] D92321: [WebAssembly] Allow element sections for nonzero table numbers

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/WasmYAML.h:68
+  uint32_t TableNumber;
+  ValueType ElemKind;
   wasm::WasmInitExpr Offset;
----------------
sbc100 wrote:
> It would be nice to see all of the non-linker parts of this change tested by `llvm/test/MC/WebAssembly/tables.s`.  
> 
> However, I IIRC,  the reason that might be hard today is that we don't support declaring elements in the assembly syntax.   Perhaps we should take should figure out how that will work so we can land the core part here without depending on the linker to test them?
I'd still think it would be good to see at least one llvm-side test (not just the lld integration test) test for this code.


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:936
+    W->OS << ElemKind;
+  }
+
----------------
wingo wrote:
> sbc100 wrote:
> > wingo wrote:
> > > sbc100 wrote:
> > > > This is dead code no?  Since that flag cannot be set?
> > > > 
> > > > In fact all these changes to writeElemSection look unused (and therefore untested), perhaps we can split this out and make it part of a future PR?
> > > So the tableNumber changes above are definitely used and tested.  Definitely possible to have non-zero table numbers for the indirect function table.  I could remove this HAS_ELEM_KIND bit here but let me push back gently -- we would like to test that this output is spec-conforming and also that it "makes" sense" from a LLVM point of view (e.g. can round trip through `tables.s`).  But the former is more important than the latter.  By structuring this function to look more like the [binary spec]((https://webassembly.github.io/reference-types/core/binary/modules.html#binary-elemsec)) -- e.g. checking the `HAS_TABLE_NUMBER` flag instead of checking `TableNumber` -- I have greater confidence reading the code that it does what the spec says.
> > Sure, I don't really care if you check TableNumber of HAS_TABLE_NUMBER above.
> > 
> > But these six lines seems compltely pointless and in fact that are dead code right?    This function doesn't write arbitrary tables it *only* writes the special __indirect_function_table (at least today).   Can that ever have an ElemKind?  
> Paging this back in -- this code was not dead.  See the definition for WASM_ELEM_SEGMENT_HAS_ELEM_KIND.  I have restored it here.  WDYT?
> Paging this back in -- this code was not dead.  See the definition for WASM_ELEM_SEGMENT_HAS_ELEM_KIND.  I have restored it here.  WDYT?

Oh I see.. WASM_ELEM_SEGMENT_HAS_ELEM_KIND is defined as ~SOME_OTHER_FLAG.. that seem odd at best.   Perhaps better write to little helper function `elemSegmentNeedsElemKind()` rather than trying to make that part of the enum?   Otherwise folks will think that `WASM_ELEM_SEGMENT_HAS_ELEM_KIND` is just another flag they can `|` together.   Either that or maybe add MASK to its name and take it out of the enum with the others.

It looks like WASM_ELEM_SEGMENT_HAS_ELEM_KIND is basically `PASSIVE | HAS_TABLE_NUMBER` ... maybe its more clear to just write that out here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92321