From 35808215e9c7500d61f23c9bfdb2862ff841d056 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 10 Sep 2024 00:42:16 +0000 Subject: [PATCH 01/10] [EH] Fix pop enclosed within a block in DCE #6400 fixed this case but that handled only when a `pop` is an immediate child of the current expression, but a `pop` can be nested deeper down. Fixes #6918. --- src/passes/DeadCodeElimination.cpp | 7 +-- test/lit/passes/dce-eh-legacy.wast | 78 ++++++++++++++++++++++++++++-- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/src/passes/DeadCodeElimination.cpp b/src/passes/DeadCodeElimination.cpp index f8acde7b0fc..fc062a4aac1 100644 --- a/src/passes/DeadCodeElimination.cpp +++ b/src/passes/DeadCodeElimination.cpp @@ -29,6 +29,7 @@ // #include "ir/eh-utils.h" +#include "ir/find_all.h" #include "ir/iteration.h" #include "ir/properties.h" #include "ir/type-updating.h" @@ -90,11 +91,7 @@ struct DeadCodeElimination Builder builder(*getModule()); std::vector remainingChildren; bool afterUnreachable = false; - bool hasPop = false; for (auto* child : ChildIterator(curr)) { - if (child->is()) { - hasPop = true; - } if (afterUnreachable) { typeUpdater.noteRecursiveRemoval(child); continue; @@ -110,7 +107,7 @@ struct DeadCodeElimination replaceCurrent(remainingChildren[0]); } else { replaceCurrent(builder.makeBlock(remainingChildren)); - if (hasPop) { + if (!FindAll(curr).list.empty()) { // We are moving a pop into a new block we just created, which // means we may need to fix things up here. needEHFixups = true; diff --git a/test/lit/passes/dce-eh-legacy.wast b/test/lit/passes/dce-eh-legacy.wast index ef6d569d69b..4d1adde9cf5 100644 --- a/test/lit/passes/dce-eh-legacy.wast +++ b/test/lit/passes/dce-eh-legacy.wast @@ -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) @@ -149,11 +163,69 @@ ) ) - ;; 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: (catch_all + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $pop-within-block (result i32) + (try (result i32) + (do + (i32.const 0) + ) + (catch $e-eqref + (drop + ;; This becomes after optimizations, moving 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) + ) + (catch_all + (i32.const 0) + ) + ) + ) ) From cf0fda5c6599b91b92e92f526d4d342f54375981 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 10 Sep 2024 05:03:19 +0000 Subject: [PATCH 02/10] Remove unnecessary catch_all --- test/lit/passes/dce-eh-legacy.wast | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/lit/passes/dce-eh-legacy.wast b/test/lit/passes/dce-eh-legacy.wast index 4d1adde9cf5..d62189542ce 100644 --- a/test/lit/passes/dce-eh-legacy.wast +++ b/test/lit/passes/dce-eh-legacy.wast @@ -222,9 +222,6 @@ ) (i32.const 0) ) - (catch_all - (i32.const 0) - ) ) ) ) From d84e864815ef787a30a6697b023b4961b8303107 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 10 Sep 2024 05:10:45 +0000 Subject: [PATCH 03/10] expectation update --- test/lit/passes/dce-eh-legacy.wast | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/lit/passes/dce-eh-legacy.wast b/test/lit/passes/dce-eh-legacy.wast index d62189542ce..d39737e2d95 100644 --- a/test/lit/passes/dce-eh-legacy.wast +++ b/test/lit/passes/dce-eh-legacy.wast @@ -191,9 +191,6 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (catch_all - ;; CHECK-NEXT: (i32.const 0) - ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) (func $pop-within-block (result i32) From 6a46b54525536070a9169de9a6218a84ec3bd124 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 10 Sep 2024 08:52:03 +0000 Subject: [PATCH 04/10] feature check to avoid unnecessary FindAll --- src/passes/DeadCodeElimination.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/passes/DeadCodeElimination.cpp b/src/passes/DeadCodeElimination.cpp index fc062a4aac1..74ff855aea2 100644 --- a/src/passes/DeadCodeElimination.cpp +++ b/src/passes/DeadCodeElimination.cpp @@ -107,7 +107,8 @@ struct DeadCodeElimination replaceCurrent(remainingChildren[0]); } else { replaceCurrent(builder.makeBlock(remainingChildren)); - if (!FindAll(curr).list.empty()) { + if (getModule()->features.hasExceptionHandling() && + !FindAll(curr).list.empty()) { // We are moving a pop into a new block we just created, which // means we may need to fix things up here. needEHFixups = true; From 845a73e9b728cacd91d866757f3af1ffe2710742 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 10 Sep 2024 11:03:43 -0700 Subject: [PATCH 05/10] Update test/lit/passes/dce-eh-legacy.wast Co-authored-by: Alon Zakai --- test/lit/passes/dce-eh-legacy.wast | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/lit/passes/dce-eh-legacy.wast b/test/lit/passes/dce-eh-legacy.wast index d39737e2d95..cae02deb4fc 100644 --- a/test/lit/passes/dce-eh-legacy.wast +++ b/test/lit/passes/dce-eh-legacy.wast @@ -200,7 +200,7 @@ ) (catch $e-eqref (drop - ;; This becomes after optimizations, moving the 'pop' inside a block, + ;; Optimization moves the 'pop' inside a block, ;; which needs to be extracted out of the block at the end. ;; (block ;; (drop From 41067238fb01a9f75da99a6318c532b1409c9a6b Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 10 Sep 2024 18:04:23 +0000 Subject: [PATCH 06/10] 80 col wrapping --- test/lit/passes/dce-eh-legacy.wast | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/lit/passes/dce-eh-legacy.wast b/test/lit/passes/dce-eh-legacy.wast index cae02deb4fc..107c35609cd 100644 --- a/test/lit/passes/dce-eh-legacy.wast +++ b/test/lit/passes/dce-eh-legacy.wast @@ -200,8 +200,8 @@ ) (catch $e-eqref (drop - ;; Optimization moves the 'pop' inside a block, - ;; which needs to be extracted out of the block at the end. + ;; 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 From 277f3a7a2962bdfd490a2e2567d69127e68540ba Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 10 Sep 2024 22:42:14 +0000 Subject: [PATCH 07/10] Run fixup whenever we see a pop --- src/passes/DeadCodeElimination.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/passes/DeadCodeElimination.cpp b/src/passes/DeadCodeElimination.cpp index 74ff855aea2..7e40bb5d430 100644 --- a/src/passes/DeadCodeElimination.cpp +++ b/src/passes/DeadCodeElimination.cpp @@ -57,6 +57,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) { @@ -74,6 +76,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()) { + needEHFixups = true; + } + if (!Properties::isControlFlowStructure(curr)) { // Control flow structures require special handling, but others are // simple. @@ -107,12 +116,6 @@ struct DeadCodeElimination replaceCurrent(remainingChildren[0]); } else { replaceCurrent(builder.makeBlock(remainingChildren)); - if (getModule()->features.hasExceptionHandling() && - !FindAll(curr).list.empty()) { - // We are moving a pop into a new block we just created, which - // means we may need to fix things up here. - needEHFixups = true; - } } } } @@ -194,8 +197,6 @@ struct DeadCodeElimination } } - bool needEHFixups = false; - void visitFunction(Function* curr) { if (needEHFixups) { EHUtils::handleBlockNestedPops(curr, *getModule()); From 4922a0b42736d8ca69a3dd929c3ff6378f5d6fd6 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 10 Sep 2024 22:43:46 +0000 Subject: [PATCH 08/10] Remove unnecessary include --- src/passes/DeadCodeElimination.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/passes/DeadCodeElimination.cpp b/src/passes/DeadCodeElimination.cpp index 7e40bb5d430..f7760ef0f70 100644 --- a/src/passes/DeadCodeElimination.cpp +++ b/src/passes/DeadCodeElimination.cpp @@ -29,7 +29,6 @@ // #include "ir/eh-utils.h" -#include "ir/find_all.h" #include "ir/iteration.h" #include "ir/properties.h" #include "ir/type-updating.h" From 6d7fb2117189af6778ae2da354d6da8b4975dec7 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Wed, 11 Sep 2024 00:22:38 +0000 Subject: [PATCH 09/10] hasPop && addedBlocks --- src/passes/DeadCodeElimination.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/passes/DeadCodeElimination.cpp b/src/passes/DeadCodeElimination.cpp index f7760ef0f70..d13e206003c 100644 --- a/src/passes/DeadCodeElimination.cpp +++ b/src/passes/DeadCodeElimination.cpp @@ -56,7 +56,10 @@ struct DeadCodeElimination // as we remove code, we must keep the types of other nodes valid TypeUpdater typeUpdater; - bool needEHFixups = false; + // Information used to decide whether we need EH fixups at the end + bool hasPop = false; // Do we have a 'pop' in this function? + bool addedBlock = false; // Have we added blocks in this function? + Expression* replaceCurrent(Expression* expression) { auto* old = getCurrent(); @@ -75,11 +78,8 @@ 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()) { - needEHFixups = true; + hasPop = true; } if (!Properties::isControlFlowStructure(curr)) { @@ -114,6 +114,7 @@ struct DeadCodeElimination if (remainingChildren.size() == 1) { replaceCurrent(remainingChildren[0]); } else { + addedBlock = true; replaceCurrent(builder.makeBlock(remainingChildren)); } } @@ -197,7 +198,10 @@ struct DeadCodeElimination } void visitFunction(Function* curr) { - if (needEHFixups) { + // We conservatively run the EH pop fixup if this function has a 'pop' and + // if we have ever added blocks in the optimization, which may have moved + // pops into the blocks. + if (hasPop && addedBlock) { EHUtils::handleBlockNestedPops(curr, *getModule()); } } From 92153697afaa2bcf80cc64955a1b56057aa9d540 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Wed, 11 Sep 2024 00:24:22 +0000 Subject: [PATCH 10/10] clang-format --- src/passes/DeadCodeElimination.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/passes/DeadCodeElimination.cpp b/src/passes/DeadCodeElimination.cpp index d13e206003c..33a163756be 100644 --- a/src/passes/DeadCodeElimination.cpp +++ b/src/passes/DeadCodeElimination.cpp @@ -57,10 +57,9 @@ struct DeadCodeElimination TypeUpdater typeUpdater; // Information used to decide whether we need EH fixups at the end - bool hasPop = false; // Do we have a 'pop' in this function? + bool hasPop = false; // Do we have a 'pop' in this function? bool addedBlock = false; // Have we added blocks in this function? - Expression* replaceCurrent(Expression* expression) { auto* old = getCurrent(); if (old == expression) {