[PATCH] D96001: [WebAssembly][lld] Preassign table number 0 to indirect function table for MVP inputs

[Sam Clegg via Phabricator via llvm-commits]

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

Nice!  I like this much better now.  Thanks for making all those changes.

Just a few more nits and I think we can land this.



================
Comment at: lld/test/wasm/invalid-mvp-table-use.s:3
+#
+# Can't define tables in MVP files.
+# RUN: not wasm-ld --no-entry %t.o -o %t.wasm 2>&1 | FileCheck -check-prefix=CHECK-ERR %s
----------------
Can you elaborate on this comment.  What makes this an MVP file ?  Is it simply the lack of `-mattr=+reference-types`.

Not part of this PR, but should we have llvm-mc fail here to catch this earlier?


================
Comment at: lld/wasm/InputFiles.cpp:375
   }
-  for (const auto &table : tables) {
-    auto *info = make<llvm::wasm::WasmSymbolInfo>();
-    // Empty name.
-    info->Kind = WASM_SYMBOL_TYPE_TABLE;
-    info->Flags = WASM_SYMBOL_BINDING_LOCAL;
-    info->Flags |= WASM_SYMBOL_VISIBILITY_HIDDEN;
-    info->Flags |= WASM_SYMBOL_NO_STRIP;
-    info->ElementIndex = tableNumber++;
-    LLVM_DEBUG(dbgs() << "Synthesizing symbol for table definition: "
-                      << info->Name << "\n");
-    auto *wasmSym = make<WasmSymbol>(*info, globalType, &table->getType(),
-                                     eventType, signature);
-    symbols.push_back(createDefined(*wasmSym));
-    // Mark live, for the same reasons as for imported tables.
-    symbols.back()->markLive();
+  assert(tableImport);
+
----------------
Can we replace this above block with simply: 


================
Comment at: lld/wasm/InputFiles.h:161
+  void
+  synthesizeMVPIndirectFunctionTableSymbolIfNeeded(uint32_t tableSymbolCount);
 
----------------
Can we call this addLegacyIndirectFunctionTableSymbolIfNeeded now so matches the config name?


================
Comment at: lld/wasm/SyntheticSections.cpp:210
 
+void TableSection::addTable(InputTable *table) {
+  if (!table->live)
----------------
nit: Moving these method back down below writeBody would make the diff easier to read (smaller).


================
Comment at: lld/wasm/SyntheticSections.cpp:217
+    if (auto *sym = dyn_cast<DefinedTable>(WasmSym::indirectFunctionTable)) {
+      if (sym->table == table) {
+        if (out.importSec->getNumImportedTables()) {
----------------
Can we combine these two into a single condition to remove one level of testing?   In fact can we combine all three maybe?


================
Comment at: lld/wasm/Writer.cpp:580
+  if (WasmSym::indirectFunctionTable) {
+    if (shouldImport(WasmSym::indirectFunctionTable))
+      out.importSec->addImport(WasmSym::indirectFunctionTable);
----------------
This can be combined into single condition (with no braces).


================
Comment at: lld/wasm/Writer.cpp:587
       continue;
-    if (!sym->isUsedInRegularObj)
+    if (sym == WasmSym::indirectFunctionTable)
       continue;
----------------
Oh nice so we always import this first, regardless of the config flag.. I like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96001