Skip to content

Commit bd1e685

Browse files
authored
[Metadce] Report removed imports due to RemoveUnusedModuleElements (#7748)
wasm-metadce does a graph analysis to find unreached things, and then cleans up using RemoveUnusedModuleElements. That pass become more powerful in #7728, which led to a situation where an import was removed from the wasm, but wasm-metadce did not report that it had removed it. This led to unneeded code in the JS (it kept sending that import, unnecessarily). This was a harmless minor waste of JS size, but it did cause a test error on Emscripten (#7747), as it parses that JS to check some things, and it found an import in JS without a use in wasm. To fix that, check if that pass removed imports, and report them.
1 parent 9570477 commit bd1e685

File tree

3 files changed

+107
-1
lines changed

3 files changed

+107
-1
lines changed

src/tools/wasm-metadce.cpp

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,12 @@ struct MetaDCEGraph {
8989
// import module.base => DCE name
9090
std::unordered_map<Name, Name> importIdToDCENode;
9191

92+
// import DCE name => items in the wasm { kind, internal name }
93+
// (a vector is needed here as an import from the outside may be imported
94+
// multiple times inside the wasm, and we can only remove it from the
95+
// outside if all wasm uses go away)
96+
std::unordered_map<Name, std::vector<KindName>> DCENodeToImports;
97+
9298
Module& wasm;
9399

94100
MetaDCEGraph(Module& wasm) : wasm(wasm) {}
@@ -112,9 +118,16 @@ struct MetaDCEGraph {
112118
ModuleUtils::iterModuleItems(wasm, [&](ModuleItemKind kind, Named* item) {
113119
if (auto* import = wasm.getImportOrNull(kind, item->name)) {
114120
auto id = getImportId(import->module, import->base);
115-
if (importIdToDCENode.find(id) == importIdToDCENode.end()) {
121+
auto iter = importIdToDCENode.find(id);
122+
if (iter == importIdToDCENode.end()) {
123+
// This is a new import, not mentioned in the graph we were given
124+
// (i.e., this import was not referred to from outside the wasm).
116125
auto dceName = getName("importId", import->name.toString());
117126
importIdToDCENode[id] = dceName;
127+
DCENodeToImports[dceName].push_back({kind, item->name});
128+
} else {
129+
// This is an existing import, mentioned in the outside graph.
130+
DCENodeToImports[iter->second].push_back({kind, item->name});
118131
}
119132
return;
120133
}
@@ -319,6 +332,46 @@ struct MetaDCEGraph {
319332
// removing functions may alter the optimum order, as # of calls can change
320333
passRunner.add("reorder-functions");
321334
passRunner.run();
335+
336+
// Standard optimizations might succeed in removing even more than our own
337+
// analysis found. That is, we build a graph of connections between things
338+
// and find which are not reached, but --remove-unused-module-elements can
339+
// use detailed understanding of wasm semantics (like how call_indirect
340+
// signatures work, traps-never-happen, etc.) which can lead to even more
341+
// things vanishing. Anything it removes, we can remove from our graph.
342+
//
343+
// The only things of interest are imports: exports are not removed by that
344+
// pass, but imports might no longer have any uses. To find imports that
345+
// were removed, scan the nodes and see what is no longer in the module.
346+
for (auto& [_, dceName] : importIdToDCENode) {
347+
auto iter = DCENodeToImports.find(dceName);
348+
if (iter == DCENodeToImports.end()) {
349+
// This appears in the graph, but did not even begin in the wasm. That
350+
// is, the outside was sending it to the wasm, but the wasm never
351+
// imported it, which means the graph was not very optimized. Just
352+
// ignore this.
353+
continue;
354+
}
355+
356+
// If all uses of this import went away, we can remove it.
357+
bool used = false;
358+
for (auto [kind, internalName] : iter->second) {
359+
// Only function imports are important here, as we do things like
360+
// generate minification maps for them, etc., but we could add others as
361+
// well.
362+
// TODO: use something like iterImportable, abstracted over
363+
// ExternalKind, to get*OrNull(), and to remove*().
364+
if (kind != ModuleItemKind::Function ||
365+
wasm.getFunctionOrNull(internalName)) {
366+
used = true;
367+
break;
368+
}
369+
}
370+
if (!used) {
371+
// This was removed from the wasm. Remove it from the graph.
372+
reached.erase(dceName);
373+
}
374+
}
322375
}
323376

324377
// Print out everything we found is not used, and so can be
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
;; RUN: wasm-metadce %s --graph-file %s.json -all -S -o - | filecheck %s
2+
3+
;; A testcase where after metadce removes things from the graph,
4+
;; remove-unused-module-elements (which is run internally) manages to remove an
5+
;; additional import. We should report that import is removed as well.
6+
7+
;; The export "used" is used, based on the graph file, while the other export
8+
;; "unused" is not. Metadce itself can remove $unused. After that,
9+
;; remove-unused-module-elements sees that no call_indirect exists that can
10+
;; reach $A, even though it is in the table, and we can remove $A. Removing
11+
;; $A then removes the import, which we should report.
12+
(module
13+
(type $A (func))
14+
15+
(import "module" "base" (func $import))
16+
17+
(table $t 60 60 funcref)
18+
19+
(elem $elem (table $t) (i32.const 0) func $A)
20+
21+
(func $used (export "used")
22+
(drop
23+
(i32.const 42)
24+
)
25+
)
26+
27+
(func $unused (export "unused")
28+
(call_indirect $t (type $A)
29+
(i32.const -1)
30+
)
31+
)
32+
33+
(func $A (type $A)
34+
(call $import)
35+
)
36+
)
37+
38+
;; CHECK: unused: export$unused
39+
;; CHECK: unused: importId$import
40+
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
[
2+
{
3+
"name": "root",
4+
"reaches": [
5+
"used"
6+
],
7+
"root": true
8+
},
9+
{
10+
"name": "used",
11+
"export": "used"
12+
}
13+
]

0 commit comments

Comments
 (0)