[PATCH] D105519: [WebAssembly] Deduplicate imports of the same module name, field name, and type

[Nick Fitzgerald via Phabricator via llvm-commits]

fitzgen added inline comments.


================
Comment at: lld/test/wasm/duplicate-function-imports.ll:1
+; RUN: llc -filetype=obj %s -o %t.o
+; RUN: wasm-ld -o %t.wasm %t.o
----------------
sbc100 wrote:
> Why `.ll` and not `.s` here? 
I used `.ll` because it was the first test I wrote and it was derived from the C snippet. I'm happy to change it to `.s` if you prefer.


================
Comment at: lld/test/wasm/duplicate-global-imports.s:30
+_start:
+  .functype _start () -> ()
+  end_function
----------------
sbc100 wrote:
> Maybe we should do a `global.get` on each of these globals so we can see that the de-duplication is working in the relocations.  
> 
> Sadly obj2yaml doesn't do disassembly so the test will be a little opaque, at least until https://reviews.llvm.org/D105539 lands
Regarding this and removing `--no-gc-sections`: is it generally preferred to create uses of things rather than to pass `--no-gc-sections` in tests?

Happy to update this either way, just curious.


================
Comment at: lld/test/wasm/duplicate-table-imports.s:2
+# RUN: llvm-mc -mattr=+reference-types -triple=wasm32-unknown-unknown -filetype=obj -o %t.o %s
+# RUN: wasm-ld --no-entry --allow-undefined --export=read_externref --export=read_funcref_1 --export=read_funcref_2 -o %t1.wasm %t.o
+# RUN: obj2yaml %t1.wasm | FileCheck %s
----------------
sbc100 wrote:
> Can you remove `--allow-undefined` ?   And maybe you can `--export-all` rather then exporting each one ?
Will do.


================
Comment at: lld/test/wasm/duplicate-table-imports.s:40
+  i32.const 0
+  table.get t3
+  end_function
----------------
sbc100 wrote:
> Lets do a `.get` on each of the tables?
There is a `table.get` for each table:

* `read_funcref_1` does it for `t1`
* `read_funcref_2` does it for `t2`
* `read_externref` does it for `t3`


================
Comment at: lld/wasm/SyntheticSections.h:198
+  llvm::DenseMap<ImportKey<WasmSignature>, uint32_t> alreadyImportedFunctions;
+  llvm::DenseMap<ImportKey<WasmTableType>, uint32_t> alreadyImportedTables;
 };
----------------
sbc100 wrote:
> Maybe just `importedGlobals`, `importedFunctions` and `importedTables`?
Sure thing, will change to these names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105519