[PATCH] D91637: [WebAssembly] Only emit indirect function table import if needed

[Andy Wingo via Phabricator via llvm-commits]

wingo added a comment.

In D91637#2400474 <https://reviews.llvm.org/D91637#2400474>, @sbc100 wrote:

> What I was hoping for here is minimal change to the object writer to remove the table from object files that don't reference it.  This seem separable from the question of whether the indirection function table should be first class MC symbol .. although maybe I'm taking this CL splitting thing to far.

Just backing up a bit for context -- the end goal to add support to wasm-ld for linking object files that reference tables; pruning needless `__indirect_function_table` imports is a "side quest".  To link tables, I need relocs for all table references written to object files, and linker symtab entries for all tables.  Do you agree that a first-class `MCSymbol` is a part of that final pipeline?  If so, I think we're on the good path here, and that using a different mechanism would just require more testing when we put the MCSymbol mechanism in place.  Anyway, just a gently argument that it seems to me that this is about the right patch size, though whether we have the symbol creation in MCContext is another question of course.



================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:506
+    BaseSym->setUsedInReloc();
+    Asm.registerSymbol(*BaseSym);
+  }
----------------
sbc100 wrote:
> Can we just move the code below, or make it conditional to avoid the MCConteext changes?
> 
>  (i.e. can we make this change local to this file?)
> 
> 
We certainly can.  However, with the next patch, we will need to call `getOrCreateWasmDefaultFunctionTableSymbol` from three places in `Target/WebAssembly`: the asm parser and from the two instruction selectors.  Do you prefer to duplicate this code there?


================
Comment at: llvm/test/MC/WebAssembly/weak-alias.s:242
 # CHECK-NEXT:         Alignment:       3
-# CHECK-NEXT:         Flags:           [ ]
 # CHECK-NEXT: ...
----------------
sbc100 wrote:
> Why did these changes occur?  Do you have some tool to automatically updating the expected output?  
I just checked and these changes are unnecessary.    I will remove this change from this patch.

What is going on here fwiw is that the weak-alias test will change when table numbers get encoded as 5-byte relocs, so this file was on the changeset for the TABLE_NUMBER patch.  I updated that test by pasting in the output of the obj2yaml with the prefix.  That's where these extraneous bits come in.

When I removed the test changes corresponding to the TABLE_NUMBER patch, we're just left with this meaningless update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91637