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

Avoid adding new unneeded names to blocks in text roundtripping #4943

Merged
merged 13 commits into from
Aug 22, 2022
Merged

Avoid adding new unneeded names to blocks in text roundtripping #4943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5d40988
yolo
kripken Aug 19, 2022
05350f5
fix
kripken Aug 19, 2022
1171241
fix
kripken Aug 19, 2022
64738b7
comment
kripken Aug 19, 2022
9eba40c
undo
kripken Aug 19, 2022
7aeb2e1
test
kripken Aug 19, 2022
f5a1452
fix
kripken Aug 19, 2022
2ffe49d
fix
kripken Aug 19, 2022
547e853
fix
kripken Aug 19, 2022
8c51239
fix
kripken Aug 19, 2022
ed3cd79
Merge remote-tracking branch 'origin/main' into s-name
kripken Aug 22, 2022
1088ffa
fix
kripken Aug 22, 2022
5ea297b
more test updates
kripken Aug 22, 2022
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
32 changes: 26 additions & 6 deletions src/wasm/wasm-s-parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1503,23 +1503,33 @@ Expression* SExpressionWasmBuilder::makeBlock(Element& s) {
// incredibly deep
auto curr = allocator.alloc<Block>();
auto* sp = &s;
std::vector<std::pair<Element*, Block*>> stack;
// The information we need for the stack of blocks here is the element we are
// converting, the block we are converting it to, and whether it originally
// had a name or not (which will be useful later).
struct Info {
Element* element;
Block* block;
bool hadName;
};
std::vector<Info> stack;
while (1) {
stack.emplace_back(sp, curr);
auto& s = *sp;
Index i = 1;
Name sName;
bool hadName = false;
if (i < s.size() && s[i]->isStr()) {
// could be a name or a type
if (s[i]->dollared() ||
stringToType(s[i]->str(), true /* allowError */) == Type::none) {
sName = s[i++]->str();
hadName = true;
} else {
sName = "block";
}
} else {
sName = "block";
}
stack.emplace_back(Info{sp, curr, hadName});
curr->name = nameMapper.pushLabelName(sName);
// block signature
curr->type = parseOptionalResultType(s, i);
Expand All @@ -1540,8 +1550,9 @@ Expression* SExpressionWasmBuilder::makeBlock(Element& s) {
}
// we now have a stack of Blocks, with their labels, but no contents yet
for (int t = int(stack.size()) - 1; t >= 0; t--) {
auto* sp = stack[t].first;
auto* curr = stack[t].second;
auto* sp = stack[t].element;
auto* curr = stack[t].block;
auto hadName = stack[t].hadName;
auto& s = *sp;
size_t i = 1;
if (i < s.size()) {
Expand All @@ -1553,7 +1564,7 @@ Expression* SExpressionWasmBuilder::makeBlock(Element& s) {
}
if (t < int(stack.size()) - 1) {
// first child is one of our recursions
curr->list.push_back(stack[t + 1].second);
curr->list.push_back(stack[t + 1].block);
i++;
}
for (; i < s.size(); i++) {
Expand All @@ -1562,8 +1573,17 @@ Expression* SExpressionWasmBuilder::makeBlock(Element& s) {
}
nameMapper.popLabelName(curr->name);
curr->finalize(curr->type);
// If the block never had a name, and one was not needed in practice (even
// if one did not exist, perhaps a break targeted it by index), then we can
// remove the name. Note that we only do this if it never had a name: if it
// did, we don't want to change anything; we just want to be the same as
// the code we are loading - if there was no name before, we don't want one
// now, so that we roundtrip text precisely.
if (!hadName && !BranchUtils::BranchSeeker::has(curr, curr->name)) {
curr->name = Name();
}
Comment on lines +1582 to +1584
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be fairly simple to avoid the quadratic behavior of ``BranchSeeker` by keeping a set of referenced names as we build branch expressions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that could be done. It would add complexity though, and this is not a hot code path - we only seek here if there never was a name. Blocks without a name tend to be rare workarounds for stacky code etc., and they do not occur in the actually dangerous situation of a tall tower of blocks for a switch - all the blocks there are named. Also, this is in the s-parser, which is not heavily optimized anyhow.

}
return stack[0].second;
return stack[0].block;
}

// Similar to block, but the label is handled by the enclosing if (since there
Expand Down
10 changes: 4 additions & 6 deletions test/binaryen.js/optimize-levels.js.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,10 @@
(memory $0 0)
(export "test" (func $test))
(func $test (param $0 i32) (result i32)
(block $block (result i32)
(if (result i32)
(local.get $0)
(local.get $0)
(i32.const 0)
)
(if (result i32)
(local.get $0)
(local.get $0)
(i32.const 0)
)
)
)
Expand Down
10 changes: 4 additions & 6 deletions test/binaryen.js/stackir.js.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,11 @@
(memory $0 0)
(export "test" (func $test))
(func $test (param $0 i32) (result i32)
block $block0 (result i32)
local.get $0
if (result i32)
local.get $0
if (result i32)
local.get $0
else
i32.const 0
end
else
i32.const 0
end
)
)
Expand Down
2 changes: 1 addition & 1 deletion test/heap-types.wast.from-wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
(ref.null $struct.A)
)
(drop
(block $block (result (ref null $struct.A))
(block (result (ref null $struct.A))
(local.get $x)
)
)
Expand Down
6 changes: 2 additions & 4 deletions test/heap-types.wast.fromBinary
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,7 @@
(ref.null $struct.A)
)
(drop
(block $label$1 (result (ref null $struct.A))
(local.get $x)
)
(local.get $x)
)
(drop
(if (result (ref null $struct.A))
Expand All @@ -83,7 +81,7 @@
)
)
(drop
(loop $label$4 (result (ref null $struct.A))
(loop $label$3 (result (ref null $struct.A))
(local.get $x)
)
)
Expand Down
6 changes: 2 additions & 4 deletions test/heap-types.wast.fromBinary.noDebugInfo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,7 @@
(ref.null ${i32_f32_f64})
)
(drop
(block $label$1 (result (ref null ${i32_f32_f64}))
(local.get $0)
)
(local.get $0)
)
(drop
(if (result (ref null ${i32_f32_f64}))
Expand All @@ -83,7 +81,7 @@
)
)
(drop
(loop $label$4 (result (ref null ${i32_f32_f64}))
(loop $label$3 (result (ref null ${i32_f32_f64}))
(local.get $0)
)
)
Expand Down
4 changes: 2 additions & 2 deletions test/lit/passes/asyncify_enable-multivalue.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,15 @@
;; CHECK-NEXT: )
;; CHECK-NEXT: (if
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: (block $block
;; CHECK-NEXT: (block
;; CHECK-NEXT: (global.set $sleeping
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (call $asyncify_start_unwind
;; CHECK-NEXT: (i32.const 4)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (block $block0
;; CHECK-NEXT: (block
;; CHECK-NEXT: (global.set $sleeping
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
Expand Down
4 changes: 2 additions & 2 deletions test/lit/passes/[email protected],env.import2.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,15 @@
;; CHECK-NEXT: )
;; CHECK-NEXT: (if
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: (block $block
;; CHECK-NEXT: (block
;; CHECK-NEXT: (global.set $sleeping
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (call $asyncify_start_unwind
;; CHECK-NEXT: (i32.const 4)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (block $block0
;; CHECK-NEXT: (block
;; CHECK-NEXT: (global.set $sleeping
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
Expand Down
6 changes: 4 additions & 2 deletions test/lit/passes/avoid-reinterprets.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (f32.reinterpret_i32
;; CHECK-NEXT: (block $block (result i32)
;; CHECK-NEXT: (block $a-name-avoids-fallthrough (result i32)
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
Expand All @@ -246,9 +246,11 @@
)
(drop
(f32.reinterpret_i32
(block (result i32)
(block $a-name-avoids-fallthrough (result i32)
(nop) ;; this would be removed by other opts, but in general, we can't
;; just look at the fallthrough, as we can't just remove code here
;; (note that we need a name on the block, or else we would look at
;; the fallthrough)
(local.get $x)
)
)
Expand Down
6 changes: 4 additions & 2 deletions test/lit/passes/avoid-reinterprets64.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (f32.reinterpret_i32
;; CHECK-NEXT: (block $block (result i32)
;; CHECK-NEXT: (block $a-name-avoids-fallthrough (result i32)
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
Expand All @@ -246,9 +246,11 @@
)
(drop
(f32.reinterpret_i32
(block (result i32)
(block $a-name-avoids-fallthrough (result i32)
(nop) ;; this would be removed by other opts, but in general, we can't
;; just look at the fallthrough, as we can't just remove code here
;; (note that we need a name on the block, or else we would look at
;; the fallthrough)
(local.get $x)
)
)
Expand Down
24 changes: 11 additions & 13 deletions test/lit/passes/catch-pop-fixup-eh.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
;; CHECK-NEXT: (pop i32)
;; CHECK-NEXT: )
;; CHECK-NEXT: (throw $e-i32
;; CHECK-NEXT: (block $block (result i32)
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (local.get $0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
Expand Down Expand Up @@ -60,11 +60,11 @@
;; CHECK-NEXT: (pop i32)
;; CHECK-NEXT: )
;; CHECK-NEXT: (throw $e-i32
;; CHECK-NEXT: (block $block (result i32)
;; CHECK-NEXT: (block $block0 (result i32)
;; CHECK-NEXT: (block $block1 (result i32)
;; CHECK-NEXT: (block $block2 (result i32)
;; CHECK-NEXT: (block $block3 (result i32)
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (local.get $0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
Expand Down Expand Up @@ -177,12 +177,10 @@
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: )
;; CHECK-NEXT: (catch $e-i32
;; CHECK-NEXT: (block $block
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (pop i32)
;; CHECK-NEXT: )
;; CHECK-NEXT: (call $helper)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (pop i32)
;; CHECK-NEXT: )
;; CHECK-NEXT: (call $helper)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
Expand Down Expand Up @@ -331,7 +329,7 @@
;; CHECK-NEXT: (pop i32 f32)
;; CHECK-NEXT: )
;; CHECK-NEXT: (throw $e-i32
;; CHECK-NEXT: (block $block (result i32)
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: )
Expand Down Expand Up @@ -367,7 +365,7 @@
;; CHECK-NEXT: (pop (ref $struct.A))
;; CHECK-NEXT: )
;; CHECK-NEXT: (throw $e-struct.A
;; CHECK-NEXT: (block $block (result (ref $struct.A))
;; CHECK-NEXT: (block (result (ref $struct.A))
;; CHECK-NEXT: (ref.as_non_null
;; CHECK-NEXT: (local.get $0)
;; CHECK-NEXT: )
Expand Down
10 changes: 4 additions & 6 deletions test/lit/passes/coalesce-locals.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1966,12 +1966,10 @@
)
;; CHECK: (func $nop-in-unreachable
;; CHECK-NEXT: (local $0 i32)
;; CHECK-NEXT: (block $block
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (i32.store
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (i32.store
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $nop-in-unreachable
Expand Down
32 changes: 11 additions & 21 deletions test/lit/passes/code-folding_enable-threads.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,20 +114,18 @@
;; CHECK-NEXT: (block $label$A
;; CHECK-NEXT: (if
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (block $block
;; CHECK-NEXT: (block $block0
;; CHECK-NEXT: (block $label$B
;; CHECK-NEXT: (if
;; CHECK-NEXT: (block
;; CHECK-NEXT: (block $label$B
;; CHECK-NEXT: (if
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (br_table $label$A $label$B
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (br_table $label$A $label$B
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (return)
;; CHECK-NEXT: )
;; CHECK-NEXT: (return)
;; CHECK-NEXT: )
;; CHECK-NEXT: (block $block2
;; CHECK-NEXT: (block
;; CHECK-NEXT: (block $label$C
;; CHECK-NEXT: (if
;; CHECK-NEXT: (unreachable)
Expand Down Expand Up @@ -177,14 +175,10 @@
;; CHECK-NEXT: (block $label$A
;; CHECK-NEXT: (if
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (block $block
;; CHECK-NEXT: (block $block4
;; CHECK-NEXT: (br $folding-inner0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (block $block6
;; CHECK-NEXT: (block
;; CHECK-NEXT: (br $folding-inner0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (br $folding-inner0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
Expand Down Expand Up @@ -319,17 +313,13 @@
;; CHECK-NEXT: )
;; CHECK-NEXT: (if
;; CHECK-NEXT: (global.get $global$0)
;; CHECK-NEXT: (block $block
;; CHECK-NEXT: (br $folding-inner0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (br $folding-inner0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: (if
;; CHECK-NEXT: (global.get $global$0)
;; CHECK-NEXT: (block $block1
;; CHECK-NEXT: (br $folding-inner0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (br $folding-inner0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
Expand Down
4 changes: 2 additions & 2 deletions test/lit/passes/dae-gc-refine-params.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block $block (result (ref null ${i32}))
;; CHECK-NEXT: (block (result (ref null ${i32}))
;; CHECK-NEXT: (local.tee $x
;; CHECK-NEXT: (call $get_null_{i32_i64})
;; CHECK-NEXT: )
Expand All @@ -322,7 +322,7 @@
;; NOMNL-NEXT: (local.get $x)
;; NOMNL-NEXT: )
;; NOMNL-NEXT: (drop
;; NOMNL-NEXT: (block $block (result (ref null ${i32}))
;; NOMNL-NEXT: (block (result (ref null ${i32}))
;; NOMNL-NEXT: (local.tee $x
;; NOMNL-NEXT: (call $get_null_{i32_i64})
;; NOMNL-NEXT: )
Expand Down
Loading