From 3d394018fe30e5d7ea153c975a158b95c1720393 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 25 Nov 2024 09:33:10 -0800 Subject: [PATCH 1/2] [GC] Refinalize after selectify in RemoveUnusedBrs (#7104) Replacing an if with a select may have refined the type. Without this fix, the sharper stale type checks complain. --- src/passes/RemoveUnusedBrs.cpp | 14 +++++++++-- test/lit/passes/remove-unused-brs-gc.wast | 29 +++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp index 96db281d87f..44549f68dff 100644 --- a/src/passes/RemoveUnusedBrs.cpp +++ b/src/passes/RemoveUnusedBrs.cpp @@ -1146,6 +1146,7 @@ struct RemoveUnusedBrs : public WalkerPass> { PassOptions& passOptions; bool needUniqify = false; + bool refinalize = false; FinalOptimizer(PassOptions& passOptions) : passOptions(passOptions) {} @@ -1419,8 +1420,14 @@ struct RemoveUnusedBrs : public WalkerPass> { if (condition.invalidates(ifTrue) || condition.invalidates(ifFalse)) { return nullptr; } - return Builder(*getModule()) - .makeSelect(iff->condition, iff->ifTrue, iff->ifFalse); + auto* select = Builder(*getModule()) + .makeSelect(iff->condition, iff->ifTrue, iff->ifFalse); + if (select->type != iff->type) { + // If the select is more refined than the if it replaces, we must + // propagate that outwards. + refinalize = true; + } + return select; } void visitLocalSet(LocalSet* curr) { @@ -1793,6 +1800,9 @@ struct RemoveUnusedBrs : public WalkerPass> { if (finalOptimizer.needUniqify) { wasm::UniqueNameMapper::uniquify(func->body); } + if (finalOptimizer.refinalize) { + ReFinalize().walkFunctionInModule(func, getModule()); + } } }; diff --git a/test/lit/passes/remove-unused-brs-gc.wast b/test/lit/passes/remove-unused-brs-gc.wast index 0a9e0885873..fa7a6d72739 100644 --- a/test/lit/passes/remove-unused-brs-gc.wast +++ b/test/lit/passes/remove-unused-brs-gc.wast @@ -864,4 +864,33 @@ ) ) ) + + ;; CHECK: (func $select-refinalize (type $13) (param $param (ref $struct)) (result (ref struct)) + ;; CHECK-NEXT: (select (result (ref $struct)) + ;; CHECK-NEXT: (select (result (ref $struct)) + ;; CHECK-NEXT: (struct.new_default $struct) + ;; CHECK-NEXT: (struct.new_default $struct) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.get $param) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $select-refinalize (param $param (ref $struct)) (result (ref struct)) + ;; The inner if can turn into a select. The type then changes, allowing the + ;; outer select to be refined, which will error if we do not refinalize. + (select (result (ref struct)) + (if (result (ref struct)) + (i32.const 0) + (then + (struct.new_default $struct) + ) + (else + (struct.new_default $struct) + ) + ) + (local.get $param) + (i32.const 0) + ) + ) ) From a2a8d2a3a067a23c547b51b4544a933f77a1c03c Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Mon, 25 Nov 2024 12:05:15 -0800 Subject: [PATCH 2/2] Handle unoptimized branches in CodeFolding (#7111) CodeFolding previously did not consider br_on_* instructions at all, so it would happily merge tails even if there were br_on_* branches to the same label with non-matching tails. Fix the bug by making any label targeted by any instruction not explicitly handled by CodeFolding unoptimizable. This will gracefully handle other branching instructions like `resume` and `resume_throw` as well. Folding these branches properly is left as future work. Also rename the test file from code-folding_enable-threads.wast to just code-folding.wast and enable all features instead of just threads. The old name was left over from when the test was originally ported to lit, and the new feature is necessary because the new test uses GC instructions. --- src/passes/CodeFolding.cpp | 22 ++++--- ..._enable-threads.wast => code-folding.wast} | 63 +++++++++++++++---- 2 files changed, 65 insertions(+), 20 deletions(-) rename test/lit/passes/{code-folding_enable-threads.wast => code-folding.wast} (87%) diff --git a/src/passes/CodeFolding.cpp b/src/passes/CodeFolding.cpp index 21527da6b18..0cddec4ca3f 100644 --- a/src/passes/CodeFolding.cpp +++ b/src/passes/CodeFolding.cpp @@ -84,7 +84,9 @@ struct ExpressionMarker void visitExpression(Expression* expr) { marked.insert(expr); } }; -struct CodeFolding : public WalkerPass> { +struct CodeFolding + : public WalkerPass< + ControlFlowWalker>> { bool isFunctionParallel() override { return true; } std::unique_ptr create() override { @@ -138,6 +140,17 @@ struct CodeFolding : public WalkerPass> { // walking + void visitExpression(Expression* curr) { + // For any branching instruction not explicitly handled by this pass, mark + // the labels it branches to unoptimizable. + // TODO: Handle folding br_on* instructions. br_on_null could be folded with + // other kinds of branches and br_on_non_null, br_on_cast, and + // br_on_cast_fail instructions could be folded with other copies of + // themselves. + BranchUtils::operateOnScopeNameUses( + curr, [&](Name label) { unoptimizables.insert(label); }); + } + void visitBreak(Break* curr) { if (curr->condition || curr->value) { unoptimizables.insert(curr->name); @@ -155,13 +168,6 @@ struct CodeFolding : public WalkerPass> { } } - void visitSwitch(Switch* curr) { - for (auto target : curr->targets) { - unoptimizables.insert(target); - } - unoptimizables.insert(curr->default_); - } - void visitUnreachable(Unreachable* curr) { // we can only optimize if we are at the end of the parent block if (!controlFlowStack.empty()) { diff --git a/test/lit/passes/code-folding_enable-threads.wast b/test/lit/passes/code-folding.wast similarity index 87% rename from test/lit/passes/code-folding_enable-threads.wast rename to test/lit/passes/code-folding.wast index b07000278ed..35816748149 100644 --- a/test/lit/passes/code-folding_enable-threads.wast +++ b/test/lit/passes/code-folding.wast @@ -1,7 +1,7 @@ ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. ;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up. -;; RUN: foreach %s %t wasm-opt --code-folding --enable-threads -S -o - | filecheck %s +;; RUN: foreach %s %t wasm-opt -all --code-folding -S -o - | filecheck %s (module ;; CHECK: (type $0 (func)) @@ -15,13 +15,13 @@ (memory $0 1 1) ;; CHECK: (table $0 282 282 funcref) - ;; CHECK: (func $0 + ;; CHECK: (func $0 (type $0) ;; CHECK-NEXT: (block $label$1 ;; CHECK-NEXT: (if ;; CHECK-NEXT: (i32.const 1) ;; CHECK-NEXT: (then ;; CHECK-NEXT: (block $label$3 - ;; CHECK-NEXT: (call_indirect (type $13) + ;; CHECK-NEXT: (call_indirect $0 (type $13) ;; CHECK-NEXT: (block $label$4 ;; CHECK-NEXT: (br $label$3) ;; CHECK-NEXT: ) @@ -52,7 +52,7 @@ ) ) ) - ;; CHECK: (func $negative-zero (result f32) + ;; CHECK: (func $negative-zero (type $1) (result f32) ;; CHECK-NEXT: (if (result f32) ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: (then @@ -82,7 +82,7 @@ ) ) ) - ;; CHECK: (func $negative-zero-b (result f32) + ;; CHECK: (func $negative-zero-b (type $1) (result f32) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: ) @@ -105,7 +105,7 @@ ) ) ) - ;; CHECK: (func $negative-zero-c (result f32) + ;; CHECK: (func $negative-zero-c (type $1) (result f32) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: ) @@ -128,7 +128,7 @@ ) ) ) - ;; CHECK: (func $break-target-outside-of-return-merged-code + ;; CHECK: (func $break-target-outside-of-return-merged-code (type $0) ;; CHECK-NEXT: (block $label$A ;; CHECK-NEXT: (if ;; CHECK-NEXT: (unreachable) @@ -202,7 +202,7 @@ ) ) ) - ;; CHECK: (func $break-target-inside-all-good + ;; CHECK: (func $break-target-inside-all-good (type $0) ;; CHECK-NEXT: (block $folding-inner0 ;; CHECK-NEXT: (block $label$A ;; CHECK-NEXT: (if @@ -269,7 +269,7 @@ ) ) ) - ;; CHECK: (func $leave-inner-block-type + ;; CHECK: (func $leave-inner-block-type (type $0) ;; CHECK-NEXT: (block $label$1 ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block $label$2 @@ -312,7 +312,7 @@ (memory $0 1 1 shared) ;; CHECK: (export "func_2224" (func $0)) (export "func_2224" (func $0)) - ;; CHECK: (func $0 (result i32) + ;; CHECK: (func $0 (type $0) (result i32) ;; CHECK-NEXT: (local $var$0 i32) ;; CHECK-NEXT: (if (result i32) ;; CHECK-NEXT: (i32.const 0) @@ -352,7 +352,7 @@ ;; CHECK: (global $global$0 (mut i32) (i32.const 10)) (global $global$0 (mut i32) (i32.const 10)) - ;; CHECK: (func $determinism + ;; CHECK: (func $determinism (type $0) ;; CHECK-NEXT: (block $folding-inner0 ;; CHECK-NEXT: (block ;; CHECK-NEXT: (block $label$1 @@ -439,7 +439,7 @@ ) (unreachable) ) - ;; CHECK: (func $careful-of-the-switch (param $0 i32) + ;; CHECK: (func $careful-of-the-switch (type $1) (param $0 i32) ;; CHECK-NEXT: (block $label$1 ;; CHECK-NEXT: (block $label$3 ;; CHECK-NEXT: (block $label$5 @@ -482,3 +482,42 @@ ) ) ) + +(module + ;; CHECK: (type $0 (func)) + + ;; CHECK: (func $br-on-null (type $0) + ;; CHECK-NEXT: (block $block + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (br_on_null $block + ;; CHECK-NEXT: (ref.null none) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (block (result i32) + ;; CHECK-NEXT: (call $br-on-null) + ;; CHECK-NEXT: (br $block) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (call $br-on-null) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $br-on-null + (block $block + (drop + ;; The other two tails are the same, but this br_on_null should inhibit code + ;; folding. + (br_on_null $block + (ref.null none) + ) + ) + (drop + (block (result i32) + (call $br-on-null) + (br $block) + ) + ) + (call $br-on-null) + ) + ) +)