Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes regarding explicit names #6466

Merged
merged 5 commits into from
Apr 11, 2024
Merged

Fixes regarding explicit names #6466

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
44459f7
Test component names after a merge
vouillon Apr 3, 2024
e5ba9c2
wasm-merge: preserve explicit names
vouillon Apr 3, 2024
f8c6b7e
Set name as explicit when copying a function with a new name
vouillon Apr 5, 2024
bae02d1
Updates
vouillon Apr 8, 2024
7afab53
Moved back type name copy to module-utils and consider placeholder as…
vouillon Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/ir/module-splitting.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,9 @@ void ModuleSplitter::exportImportFunction(Name funcName) {
// Import the function if it is not already imported into the secondary
// module.
if (secondary.getFunctionOrNull(funcName) == nullptr) {
auto func =
Builder::makeFunction(funcName, primary.getFunction(funcName)->type, {});
auto primaryFunc = primary.getFunction(funcName);
auto func = Builder::makeFunction(funcName, primaryFunc->type, {});
func->hasExplicitName = primaryFunc->hasExplicitName;
func->module = config.importNamespace;
func->base = exportName;
secondary.addFunction(std::move(func));
Expand Down Expand Up @@ -542,7 +543,7 @@ void ModuleSplitter::setupTablePatching() {
placeholder->base = std::to_string(index);
placeholder->name = Names::getValidFunctionName(
primary, std::string("placeholder_") + placeholder->base.toString());
placeholder->hasExplicitName = false;
placeholder->hasExplicitName = true;
kripken marked this conversation as resolved.
Show resolved Hide resolved
placeholder->type = secondaryFunc->type;
kripken marked this conversation as resolved.
Show resolved Hide resolved
elem = placeholder->name;
primary.addFunction(std::move(placeholder));
Expand Down
10 changes: 9 additions & 1 deletion src/ir/module-utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ Function* copyFunction(Function* func,
std::optional<std::vector<Index>> fileIndexMap) {
auto ret = std::make_unique<Function>();
ret->name = newName.is() ? newName : func->name;
ret->hasExplicitName = func->hasExplicitName && !newName.is();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not set it to true when newName.is()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newName is a generated name. For instance, some names like outlined-A$func-name are generated during inlining. But maybe we can consider it is explicit enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you mean, good point. I guess it depends on the user expectation in this code. I think that we do want to keep such names, as we are copying a function, so we should copy the property of having an explicit name.

ret->type = func->type;
ret->vars = func->vars;
ret->localNames = func->localNames;
Expand Down Expand Up @@ -77,6 +78,7 @@ Function* copyFunction(Function* func,
Global* copyGlobal(Global* global, Module& out) {
auto* ret = new Global();
ret->name = global->name;
ret->hasExplicitName = global->hasExplicitName;
ret->type = global->type;
ret->mutable_ = global->mutable_;
ret->module = global->module;
Expand All @@ -93,6 +95,7 @@ Global* copyGlobal(Global* global, Module& out) {
Tag* copyTag(Tag* tag, Module& out) {
auto* ret = new Tag();
ret->name = tag->name;
ret->hasExplicitName = tag->hasExplicitName;
ret->sig = tag->sig;
ret->module = tag->module;
ret->base = tag->base;
Expand Down Expand Up @@ -209,6 +212,12 @@ void copyModuleItems(const Module& in, Module& out) {
for (auto& curr : in.dataSegments) {
copyDataSegment(curr.get(), out);
}

for (auto& [type, names] : in.typeNames) {
if (!out.typeNames.count(type)) {
out.typeNames[type] = names;
}
}
}

void copyModule(const Module& in, Module& out) {
Expand All @@ -222,7 +231,6 @@ void copyModule(const Module& in, Module& out) {
out.customSections = in.customSections;
out.debugInfoFileNames = in.debugInfoFileNames;
out.features = in.features;
out.typeNames = in.typeNames;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this move? The description of the function it was moved to is that it copies toplevel module items like Functions and Globals, and not metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, you think I should rather put the code that copy type names in wasm-merge.cc?

Copy link
Member

@kripken kripken Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't see why my question, which was about the deletion of this line, is answered by something about wasm-merge? What is the context I am missing?

From the standpoint of this file by itself (not thinking about wasm-merge at all), this change seems surprising as I said - it looks like this code moved up to copyModuleItems which I don't understand the motivation for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasm-merge currently does not copy type names from the input modules to the merged module. I added some code for that in copyModuleItems. But then this line became unnecessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so wasm-merge uses copyModuleItems and not copyModule. The difference between those two decreases with this change, and the meaning of the difference becomes less clear to me. Perhaps we don't need two functions here at all, and there could just be a single function with maybe a set of options as to whether to copy exports, the start method, etc. (the things only in copyModule). Anyhow, for this PR I think the change you made is good, but please add a TODO on copyModule to remind us later, maybe something like TODO: merge this with copyModuleItems, and add options for copying exports and other things that are currently different between them, if we still need those differences.

}

void clearModule(Module& wasm) {
Expand Down
32 changes: 20 additions & 12 deletions src/wasm/wasm-binary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -864,19 +864,27 @@ void WasmBinaryWriter::writeNames() {

// function names
{
auto substart =
startSubsection(BinaryConsts::CustomSections::Subsection::NameFunction);
o << U32LEB(indexes.functionIndexes.size());
Index emitted = 0;
auto add = [&](Function* curr) {
o << U32LEB(emitted);
writeEscapedName(curr->name.str);
emitted++;
std::vector<std::pair<Index, Function*>> functionsWithNames;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the very same code as for globals.

Index checked = 0;
auto check = [&](Function* curr) {
if (curr->hasExplicitName) {
functionsWithNames.push_back({checked, curr});
}
checked++;
};
ModuleUtils::iterImportedFunctions(*wasm, add);
ModuleUtils::iterDefinedFunctions(*wasm, add);
assert(emitted == indexes.functionIndexes.size());
finishSubsection(substart);
ModuleUtils::iterImportedFunctions(*wasm, check);
ModuleUtils::iterDefinedFunctions(*wasm, check);
assert(checked == indexes.functionIndexes.size());
if (functionsWithNames.size() > 0) {
auto substart =
startSubsection(BinaryConsts::CustomSections::Subsection::NameFunction);
o << U32LEB(functionsWithNames.size());
for (auto& [index, global] : functionsWithNames) {
o << U32LEB(index);
writeEscapedName(global->name.str);
}
finishSubsection(substart);
}
}

// local names
Expand Down
1 change: 1 addition & 0 deletions src/wasm/wasm-s-parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1181,6 +1181,7 @@ void SExpressionWasmBuilder::parseFunction(Element& s, bool preParseImport) {
// make a new function
currFunction = std::unique_ptr<Function>(
Builder(wasm).makeFunction(name, std::move(params), type, std::move(vars)));
currFunction->hasExplicitName = hasExplicitName;
currFunction->profile = profile;

// parse body
Expand Down
137 changes: 137 additions & 0 deletions test/lit/merge/names.wat
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
;; RUN: wasm-merge -g %s first %s.second second -all -o %t.wasm
;; RUN: wasm-opt -all %t.wasm -S -o - | filecheck %s
(module
;; CHECK: (type $0 (func))

;; CHECK: (type $t (struct (field $a i32) (field $b i32)))

;; CHECK: (type $2 (func (param (ref $t))))

;; CHECK: (type $u (struct (field $c i64) (field $d i32)))

;; CHECK: (type $4 (func (param (ref $u))))

;; CHECK: (global $global$0 i32 (i32.const 0))

;; CHECK: (global $glob2 i32 (i32.const 0))

;; CHECK: (global $global$2 i32 (i32.const 0))

;; CHECK: (global $glob0 i32 (i32.const 0))

;; CHECK: (memory $mem0 0)

;; CHECK: (memory $1 0)

;; CHECK: (memory $mem2 0)

;; CHECK: (memory $3 0)

;; CHECK: (table $table0 1 funcref)

;; CHECK: (table $1 1 funcref)

;; CHECK: (table $table2 1 funcref)

;; CHECK: (table $3 1 funcref)

;; CHECK: (tag $tag0)

;; CHECK: (tag $tag$1)

;; CHECK: (tag $tag2)

;; CHECK: (tag $tag$3)

;; CHECK: (export "m0" (memory $mem0))

;; CHECK: (export "m1" (memory $1))

;; CHECK: (export "f0" (func $func0))

;; CHECK: (export "f1" (func $1))

;; CHECK: (export "t0" (table $table0))

;; CHECK: (export "t1" (table $1))

;; CHECK: (export "g0" (global $glob0))

;; CHECK: (export "g1" (global $global$2))

;; CHECK: (export "tag0" (tag $tag0))

;; CHECK: (export "tag1" (tag $tag$1))

;; CHECK: (export "func" (func $2))

;; CHECK: (export "m2" (memory $mem2))

;; CHECK: (export "m3" (memory $3))

;; CHECK: (export "f2" (func $func2))

;; CHECK: (export "f3" (func $4))

;; CHECK: (export "t2" (table $table2))

;; CHECK: (export "t3" (table $3))

;; CHECK: (export "g2" (global $glob2))

;; CHECK: (export "g3" (global $global$0))

;; CHECK: (export "tag2" (tag $tag2))

;; CHECK: (export "tag3" (tag $tag$3))

;; CHECK: (export "func2" (func $5))

;; CHECK: (func $func0 (type $0)
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: )
(func $func0 (export "f0"))
(func (export "f1"))

(table $table0 (export "t0") 1 funcref)
(table (export "t1") 1 funcref)

(global $glob0 (export g0) i32 (i32.const 0))
(global (export g1) i32 (i32.const 0))

(memory $mem0 (export "m0") 0)
(memory (export "m1") 0)

(elem $elem0 func)
(elem func)

(data $data0 "")
(data "")

(tag $tag0 (export tag0))
(tag (export tag1))

(type $t (struct (field $a i32) (field $b i32)))

(func (export "func") (param $x (ref $t)))
)
;; CHECK: (func $1 (type $0)
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: )

;; CHECK: (func $2 (type $2) (param $x (ref $t))
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: )

;; CHECK: (func $func2 (type $0)
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: )

;; CHECK: (func $4 (type $0)
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: )

;; CHECK: (func $5 (type $4) (param $0 (ref $u))
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: )
27 changes: 27 additions & 0 deletions test/lit/merge/names.wat.second
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
(module

(func $func2 (export "f2"))
(func (export "f3"))

(table $table2 (export "t2") 1 funcref)
(table (export "t3") 1 funcref)

(memory $mem2 (export "m2") 0)
(memory (export "m3") 0)

(global $glob2 (export g2) i32 (i32.const 0))
(global (export g3) i32 (i32.const 0))

(elem $elem2 func)
(elem func)

(data $data2 "")
(data "")

(tag $tag2 (export tag2))
(tag (export tag3))

(type $u (struct (field $c i64) (field $d i32)))

(func (export "func2") (param (ref $u)))
)
4 changes: 2 additions & 2 deletions test/passes/func-metrics.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ func: func_a
Block : 1
Call : 5
start: func_a
[removable-bytes-without-it]: 57
[removable-bytes-without-it]: 60
[total] : 0
(module
(type $0 (func))
Expand Down Expand Up @@ -274,7 +274,7 @@ func: 0
[vars] : 0
GlobalGet : 1
export: stackSave (0)
[removable-bytes-without-it]: 62
[removable-bytes-without-it]: 79
[total] : 0
(module
(type $0 (func (result i32)))
Expand Down
Binary file modified test/unit/input/gc_target_feature.wasm
View file
Open in desktop
vouillon marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Contributor Author

@vouillon vouillon Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name section is no longer written when there is no name.

Binary file not shown.
Loading