[PATCH] D96001: [WebAssembly][lld] Precolor table numbers for MVP inputs

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

I'm not familiar with the use of term precoloring.   Perhaps I should be..  but maybe we could use something more common like `preassignTableNumber` rather than `precolorTableNumber`?    Certainly I don't think we want to use the term in the user-facing errors.



================
Comment at: lld/wasm/InputFiles.cpp:349
+        if (ts->getTableNumber() != tableNumber)
+          error(toString(this) + ": incompatible precoloring");
+      } else {
----------------
I'm not sure this is a very usefull user-facing error.    Can it ever actually happen?   Can we improve it or perhaps make it an assert?


================
Comment at: lld/wasm/InputFiles.cpp:351
+      } else {
+        ts->setTableNumber(tableNumber);
+      }
----------------
Can we simplify the code here by asserting that the table number is always zero and that there is only one table.

In fact, perhaps this should be called `synthesizeTableSymbol` (non-plural) and just return after for first call to `addTable`?


================
Comment at: lld/wasm/SymbolTable.cpp:213
+                                uint32_t newNumber) {
+  if (TableSymbol *ts = dyn_cast<TableSymbol>(existing)) {
+    if (!ts->hasTableNumber()) {
----------------
I think this can be an unconditionally `cast<>` can't it?


================
Comment at: lld/wasm/SymbolTable.cpp:219
+            "\n>>> assigned nonrelocatable table number " +
+            Twine(ts->getTableNumber()) + " in " + toString(ts->getFile()) +
+            "\n>>> assigned nonrelocatable table number " + Twine(newNumber) +
----------------
Similarly, since there is exactly one nonrelocatable table number (zero)... can we simplify this code based on that fact?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96001