From 533cc6c2e7b94abd7ad79b7c6702cf93250fed67 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Thu, 20 May 2021 13:08:51 -0700 Subject: [PATCH 1/4] [EH] Change Walker::TaskFunc back to function pointer (NFC) `Walker::TaskFunc` has changed from a function pointer to `std::function` in #3494, mainly to make the EH support for `CFGWalker` easier. We didn't notice much performance difference then, but it was recently reported that it creased binaryen.js code size and performance. This changes `Walker::TaskFunc` back to a function pointer and does a little more work to manage catch index in `CFGWalker` side. Hopefully fixes #3857. --- src/cfg/cfg-traversal.h | 33 ++++++++++++++++++++------------- src/wasm-traversal.h | 2 +- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/cfg/cfg-traversal.h b/src/cfg/cfg-traversal.h index 39f71dd6c8c..0ed5021ecb4 100644 --- a/src/cfg/cfg-traversal.h +++ b/src/cfg/cfg-traversal.h @@ -101,6 +101,10 @@ struct CFGWalker : public ControlFlowWalker { // but their usage does not overlap in time, and this is more efficient. std::vector> processCatchStack; + // Variable to store the catch index within catch bodies. To be used in + // doStartCatch and doEndCatch. + Index catchIndex = 0; + BasicBlock* startBasicBlock() { currBasicBlock = ((SubType*)this)->makeBasicBlock(); basicBlocks.push_back(std::unique_ptr(currBasicBlock)); @@ -306,14 +310,22 @@ struct CFGWalker : public ControlFlowWalker { self->unwindExprStack.pop_back(); } - static void doStartCatch(SubType* self, Expression** currp, Index i) { + static void incrementCatchIndex(SubType* self, Expression** currp) { + self->catchIndex++; + } + + static void resetCatchIndex(SubType* self, Expression** currp) { + self->catchIndex = 0; + } + + static void doStartCatch(SubType* self, Expression** currp) { // Get the block that starts this catch - self->currBasicBlock = self->processCatchStack.back()[i]; + self->currBasicBlock = self->processCatchStack.back()[self->catchIndex]; } - static void doEndCatch(SubType* self, Expression** currp, Index i) { + static void doEndCatch(SubType* self, Expression** currp) { // We are done with this catch; set the block that ends it - self->processCatchStack.back()[i] = self->currBasicBlock; + self->processCatchStack.back()[self->catchIndex] = self->currBasicBlock; } static void doEndTry(SubType* self, Expression** currp) { @@ -381,19 +393,14 @@ struct CFGWalker : public ControlFlowWalker { case Expression::Id::TryId: { self->pushTask(SubType::doEndTry, currp); auto& catchBodies = curr->cast()->catchBodies; - using namespace std::placeholders; for (Index i = 0; i < catchBodies.size(); i++) { - auto doEndCatchI = [i](SubType* self, Expression** currp) { - doEndCatch(self, currp, i); - }; - self->pushTask(doEndCatchI, currp); + self->pushTask(incrementCatchIndex, currp); + self->pushTask(doEndCatch, currp); self->pushTask(SubType::scan, &catchBodies[i]); - auto doStartCatchI = [i](SubType* self, Expression** currp) { - doStartCatch(self, currp, i); - }; - self->pushTask(doStartCatchI, currp); + self->pushTask(doStartCatch, currp); } self->pushTask(SubType::doStartCatches, currp); + self->pushTask(resetCatchIndex, currp); self->pushTask(SubType::scan, &curr->cast()->body); self->pushTask(SubType::doStartTry, currp); return; // don't do anything else diff --git a/src/wasm-traversal.h b/src/wasm-traversal.h index 388584fd600..eb91184e551 100644 --- a/src/wasm-traversal.h +++ b/src/wasm-traversal.h @@ -282,7 +282,7 @@ struct Walker : public VisitorType { // nested. // Tasks receive the this pointer and a pointer to the pointer to operate on - using TaskFunc = std::function; + typedef void (*TaskFunc)(SubType*, Expression**); struct Task { TaskFunc func; From 155bafb7c3059eda9914e05c494c4e836e750941 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Thu, 20 May 2021 18:49:52 -0700 Subject: [PATCH 2/4] Fix + add tests --- src/cfg/cfg-traversal.h | 19 ++++---- test/passes/rse_all-features.txt | 81 +++++++++++++++++++++++++++++++ test/passes/rse_all-features.wast | 57 ++++++++++++++++++++++ 3 files changed, 147 insertions(+), 10 deletions(-) diff --git a/src/cfg/cfg-traversal.h b/src/cfg/cfg-traversal.h index 0ed5021ecb4..cb758b15173 100644 --- a/src/cfg/cfg-traversal.h +++ b/src/cfg/cfg-traversal.h @@ -101,9 +101,9 @@ struct CFGWalker : public ControlFlowWalker { // but their usage does not overlap in time, and this is more efficient. std::vector> processCatchStack; - // Variable to store the catch index within catch bodies. To be used in + // Stack to store the catch indices within catch bodies. To be used in // doStartCatch and doEndCatch. - Index catchIndex = 0; + std::vector catchIndexStack; BasicBlock* startBasicBlock() { currBasicBlock = ((SubType*)this)->makeBasicBlock(); @@ -308,24 +308,23 @@ struct CFGWalker : public ControlFlowWalker { self->processCatchStack.push_back(self->unwindCatchStack.back()); self->unwindCatchStack.pop_back(); self->unwindExprStack.pop_back(); + self->catchIndexStack.push_back(0); } static void incrementCatchIndex(SubType* self, Expression** currp) { - self->catchIndex++; - } - - static void resetCatchIndex(SubType* self, Expression** currp) { - self->catchIndex = 0; + self->catchIndexStack.back()++; } static void doStartCatch(SubType* self, Expression** currp) { // Get the block that starts this catch - self->currBasicBlock = self->processCatchStack.back()[self->catchIndex]; + self->currBasicBlock = + self->processCatchStack.back()[self->catchIndexStack.back()]; } static void doEndCatch(SubType* self, Expression** currp) { // We are done with this catch; set the block that ends it - self->processCatchStack.back()[self->catchIndex] = self->currBasicBlock; + self->processCatchStack.back()[self->catchIndexStack.back()] = + self->currBasicBlock; } static void doEndTry(SubType* self, Expression** currp) { @@ -338,6 +337,7 @@ struct CFGWalker : public ControlFlowWalker { self->link(self->tryStack.back(), self->currBasicBlock); self->tryStack.pop_back(); self->processCatchStack.pop_back(); + self->catchIndexStack.pop_back(); } static void doEndThrow(SubType* self, Expression** currp) { @@ -400,7 +400,6 @@ struct CFGWalker : public ControlFlowWalker { self->pushTask(doStartCatch, currp); } self->pushTask(SubType::doStartCatches, currp); - self->pushTask(resetCatchIndex, currp); self->pushTask(SubType::scan, &curr->cast()->body); self->pushTask(SubType::doStartTry, currp); return; // don't do anything else diff --git a/test/passes/rse_all-features.txt b/test/passes/rse_all-features.txt index 6c9d5258568..1594425cf82 100644 --- a/test/passes/rse_all-features.txt +++ b/test/passes/rse_all-features.txt @@ -4,6 +4,7 @@ (type $i32_i32_=>_none (func (param i32 i32))) (type $i32_f64_=>_none (func (param i32 f64))) (event $e (attr 0) (param i32)) + (event $e2 (attr 0) (param)) (func $basic (param $x i32) (param $y f64) (local $a f32) (local $b i64) @@ -645,4 +646,84 @@ (i32.const 1) ) ) + (func $nested-catch1 + (local $x i32) + (try $try + (do + (throw $e + (i32.const 0) + ) + ) + (catch $e + (drop + (pop i32) + ) + ) + (catch $e2 + (try $try6 + (do + (throw $e + (i32.const 0) + ) + ) + (catch $e + (drop + (pop i32) + ) + ) + (catch $e2 + (local.set $x + (i32.const 1) + ) + ) + ) + ) + ) + (local.set $x + (i32.const 1) + ) + ) + (func $nested-catch2 + (local $x i32) + (try $try + (do + (throw $e + (i32.const 0) + ) + ) + (catch $e + (drop + (pop i32) + ) + (local.set $x + (i32.const 1) + ) + ) + (catch_all + (try $try7 + (do + (throw $e + (i32.const 0) + ) + ) + (catch $e + (drop + (pop i32) + ) + (local.set $x + (i32.const 1) + ) + ) + (catch_all + (local.set $x + (i32.const 1) + ) + ) + ) + ) + ) + (drop + (i32.const 1) + ) + ) ) diff --git a/test/passes/rse_all-features.wast b/test/passes/rse_all-features.wast index b3c684fbd85..ecf7245f878 100644 --- a/test/passes/rse_all-features.wast +++ b/test/passes/rse_all-features.wast @@ -288,6 +288,7 @@ ) (event $e (attr 0) (param i32)) + (event $e2 (attr 0)) (func $try1 (local $x i32) (try @@ -416,4 +417,60 @@ ;; so the local.set may not run. So this should NOT be dropped. (local.set $x (i32.const 1)) ) + (func $nested-catch1 + (local $x i32) + (try + (do + (throw $e (i32.const 0)) + ) + (catch $e + (drop (pop i32)) + ) + (catch $e2 + (try + (do + (throw $e (i32.const 0)) + ) + (catch $e + (drop (pop i32)) + ) + (catch $e2 + (local.set $x (i32.const 1)) + ) + ) + ) + ) + ;; This should NOT be dropped because the exception might not be caught by + ;; the inner catches, and the local.set may not run. + (local.set $x (i32.const 1)) + ) + (func $nested-catch2 + (local $x i32) + (try + (do + (throw $e (i32.const 0)) + ) + (catch $e + (drop (pop i32)) + (local.set $x (i32.const 1)) + ) + (catch_all + (try + (do + (throw $e (i32.const 0)) + ) + (catch $e + (drop (pop i32)) + (local.set $x (i32.const 1)) + ) + (catch_all + (local.set $x (i32.const 1)) + ) + ) + ) + ) + ;; This should be dropped because the exception is guaranteed to be caught + ;; by one of the catches and it will set the local to 1. + (local.set $x (i32.const 1)) + ) ) From ae4112c7a4fe60bb2ae784db9965e6eb4440ed94 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Thu, 20 May 2021 18:52:48 -0700 Subject: [PATCH 3/4] Simplify --- src/cfg/cfg-traversal.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/cfg/cfg-traversal.h b/src/cfg/cfg-traversal.h index cb758b15173..3f2eecb887c 100644 --- a/src/cfg/cfg-traversal.h +++ b/src/cfg/cfg-traversal.h @@ -311,10 +311,6 @@ struct CFGWalker : public ControlFlowWalker { self->catchIndexStack.push_back(0); } - static void incrementCatchIndex(SubType* self, Expression** currp) { - self->catchIndexStack.back()++; - } - static void doStartCatch(SubType* self, Expression** currp) { // Get the block that starts this catch self->currBasicBlock = @@ -325,6 +321,7 @@ struct CFGWalker : public ControlFlowWalker { // We are done with this catch; set the block that ends it self->processCatchStack.back()[self->catchIndexStack.back()] = self->currBasicBlock; + self->catchIndexStack.back()++; } static void doEndTry(SubType* self, Expression** currp) { @@ -394,7 +391,6 @@ struct CFGWalker : public ControlFlowWalker { self->pushTask(SubType::doEndTry, currp); auto& catchBodies = curr->cast()->catchBodies; for (Index i = 0; i < catchBodies.size(); i++) { - self->pushTask(incrementCatchIndex, currp); self->pushTask(doEndCatch, currp); self->pushTask(SubType::scan, &catchBodies[i]); self->pushTask(doStartCatch, currp); From de400d4872f4204d1ddd9d8080bdb516c4830ccf Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Thu, 20 May 2021 19:05:53 -0700 Subject: [PATCH 4/4] Update test/passes/rse_all-features.wast Co-authored-by: Alon Zakai --- test/passes/rse_all-features.wast | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/passes/rse_all-features.wast b/test/passes/rse_all-features.wast index ecf7245f878..8192ecea1f0 100644 --- a/test/passes/rse_all-features.wast +++ b/test/passes/rse_all-features.wast @@ -441,7 +441,8 @@ ) ) ;; This should NOT be dropped because the exception might not be caught by - ;; the inner catches, and the local.set may not run. + ;; the inner catches, and the local.set above us may not have run, and + ;; other possible code paths do not even set the local. (local.set $x (i32.const 1)) ) (func $nested-catch2