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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: lld/wasm/SyntheticSections.cpp:450
+    writeU8(os, elemKind, "elem kind");
+  }
+
----------------
This flag is never set so this is dead code isn't it?

I'm guessing that when we add actual multi table here we will still want to separate the linker-synthetic indirectFunctionTable from other tables that are composed of actual input segments, so I'm not sure it makes since to make this code too generic here.


================
Comment at: llvm/include/llvm/ObjectYAML/WasmYAML.h:68
+  uint32_t TableNumber;
+  ValueType ElemKind;
   wasm::WasmInitExpr Offset;
----------------
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?


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:923
+  encodeULEB128(Flags, W->OS);
+  if (Flags & wasm::WASM_ELEM_SEGMENT_HAS_TABLE_NUMBER)
+    encodeULEB128(TableNumber, W->OS); // the table number
----------------
Might as well use `if (TableNumber)` again here for consistency with above.


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:936
+    W->OS << ElemKind;
+  }
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92321