Skip to content

[EH] Fix pop enclosed within a block in DCE #6922

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

Merged
aheejin merged 10 commits into from
Sep 11, 2024
Merged

[EH] Fix pop enclosed within a block in DCE #6922

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3580821
[EH] Fix pop enclosed within a block in DCE
aheejin Sep 10, 2024
cf0fda5
Remove unnecessary catch_all
aheejin Sep 10, 2024
d84e864
expectation update
aheejin Sep 10, 2024
6a46b54
feature check to avoid unnecessary FindAll
aheejin Sep 10, 2024
845a73e
Update test/lit/passes/dce-eh-legacy.wast
aheejin Sep 10, 2024
4106723
80 col wrapping
aheejin Sep 10, 2024
277f3a7
Run fixup whenever we see a pop
aheejin Sep 10, 2024
4922a0b
Remove unnecessary include
aheejin Sep 10, 2024
6d7fb21
hasPop && addedBlocks
aheejin Sep 11, 2024
9215369
clang-format
aheejin Sep 11, 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
20 changes: 9 additions & 11 deletions src/passes/DeadCodeElimination.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ struct DeadCodeElimination
// as we remove code, we must keep the types of other nodes valid
TypeUpdater typeUpdater;

bool needEHFixups = false;

Expression* replaceCurrent(Expression* expression) {
auto* old = getCurrent();
if (old == expression) {
Expand All @@ -73,6 +75,13 @@ struct DeadCodeElimination
}

void visitExpression(Expression* curr) {
// We create new blocks in this function which can move 'pop's into blocks.
// So we conservatively run the EH fixup if we ever see a 'pop' in this
// function.
if (curr->is<Pop>()) {
needEHFixups = true;
}

if (!Properties::isControlFlowStructure(curr)) {
// Control flow structures require special handling, but others are
// simple.
Expand All @@ -90,11 +99,7 @@ struct DeadCodeElimination
Builder builder(*getModule());
std::vector<Expression*> remainingChildren;
bool afterUnreachable = false;
bool hasPop = false;
for (auto* child : ChildIterator(curr)) {
if (child->is<Pop>()) {
hasPop = true;
}
if (afterUnreachable) {
typeUpdater.noteRecursiveRemoval(child);
continue;
Expand All @@ -110,11 +115,6 @@ struct DeadCodeElimination
replaceCurrent(remainingChildren[0]);
} else {
replaceCurrent(builder.makeBlock(remainingChildren));
Copy link
Member

@kripken kripken Sep 10, 2024

Choose a reason for hiding this comment

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

The only danger with a Pop is the new block made here, so I suggest that we set a boolean addedBlock to true in this location. And we can rename needEHFixups to hasPop. Then below, the check if (needEHFixups) can be if (hasPop && addedBlock), which will be a little more precise.

Copy link
Member Author

@aheejin aheejin Sep 11, 2024

Choose a reason for hiding this comment

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

Good idea. Done.

if (hasPop) {
// We are moving a pop into a new block we just created, which
// means we may need to fix things up here.
needEHFixups = true;
}
}
}
}
Expand Down Expand Up @@ -196,8 +196,6 @@ struct DeadCodeElimination
}
}

bool needEHFixups = false;

void visitFunction(Function* curr) {
if (needEHFixups) {
EHUtils::handleBlockNestedPops(curr, *getModule());
Expand Down
72 changes: 69 additions & 3 deletions test/lit/passes/dce-eh-legacy.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,28 @@
;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited.
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
;; RUN: wasm-opt %s --dce -all -S -o - | filecheck %s

;; If either try body or catch body is reachable, the whole try construct is
;; reachable
(module
;; CHECK: (type $0 (func))

;; CHECK: (type $1 (func (param i32)))

;; CHECK: (type $2 (func (param eqref)))

;; CHECK: (type $3 (func (param i32 i32)))

;; CHECK: (type $4 (func (result i32)))

;; CHECK: (type $struct (struct (field (mut eqref))))
(type $struct (struct (field (mut (ref null eq)))))

;; CHECK: (tag $e)
(tag $e)

;; CHECK: (tag $e-i32 (param i32))
(tag $e-i32 (param i32))
;; CHECK: (tag $e-eqref (param eqref))
(tag $e-eqref (param (ref null eq)))

;; CHECK: (func $foo (type $0)
;; CHECK-NEXT: (nop)
Expand Down Expand Up @@ -149,11 +163,63 @@
)
)

;; CHECK: (func $helper-i32-i32 (type $2) (param $0 i32) (param $1 i32)
;; CHECK: (func $helper-i32-i32 (type $3) (param $0 i32) (param $1 i32)
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: )
(func $helper-i32-i32 (param $0 i32) (param $1 i32)
(nop)
)

;; CHECK: (func $pop-within-block (type $4) (result i32)
;; CHECK-NEXT: (local $0 eqref)
;; CHECK-NEXT: (try (result i32)
;; CHECK-NEXT: (do
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (catch $e-eqref
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (pop eqref)
;; CHECK-NEXT: )
;; CHECK-NEXT: (block
;; CHECK-NEXT: (block
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.new $struct
;; CHECK-NEXT: (local.get $0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $pop-within-block (result i32)
(try (result i32)
(do
(i32.const 0)
)
(catch $e-eqref
(drop
;; Optimization moves the 'pop' inside a block, which needs to be
;; extracted out of the block at the end.
;; (block
;; (drop
;; (struct.new $struct.0
;; (pop eqref)
;; )
;; )
;; (unreachable)
;; )
(ref.eq
(struct.new $struct
(pop (ref null eq))
)
(unreachable)
)
)
(i32.const 0)
)
)
)
)