[PATCH] D96001: [WebAssembly][lld] Precolor table numbers for MVP inputs
Sam Clegg via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 4 12:43:36 PST 2021
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
More information about the llvm-commits
mailing list