From 70191cfd7c82f4ccabdb0346deb3557885bbcf70 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Wed, 20 May 2026 17:09:48 -0700 Subject: [PATCH] MergeBlocks: Migrate from invalidates to orderedBefore Replace the coarse 'invalidates' check in 'MergeBlocks' with 'orderedBefore'. This allows 'MergeBlocks' to move expressions (like GC reads) out of blocks past other sibling expressions that have been left behind. Also clean up dead code in 'optimize()' by removing the unused 'dependency1' and 'dependency2' parameters and their checks (which were the other two replaced 'invalidates' calls that are no longer active). TAG=agy --- src/passes/MergeBlocks.cpp | 24 +----- test/lit/passes/merge-blocks-atomics.wast | 91 +++++++++++++++++++++++ 2 files changed, 94 insertions(+), 21 deletions(-) create mode 100644 test/lit/passes/merge-blocks-atomics.wast diff --git a/src/passes/MergeBlocks.cpp b/src/passes/MergeBlocks.cpp index 3013f67ac9b..7010d6d15f7 100644 --- a/src/passes/MergeBlocks.cpp +++ b/src/passes/MergeBlocks.cpp @@ -498,29 +498,11 @@ struct MergeBlocks // ) // at which point the block is on the outside and potentially mergeable with // an outer block - Block* optimize(Expression* curr, - Expression*& child, - Block* outer = nullptr, - Expression** dependency1 = nullptr, - Expression** dependency2 = nullptr) { + Block* + optimize(Expression* curr, Expression*& child, Block* outer = nullptr) { if (!child) { return outer; } - if ((dependency1 && *dependency1) || (dependency2 && *dependency2)) { - // there are dependencies, things we must be reordered through. make sure - // no problems there - EffectAnalyzer childEffects(getPassOptions(), *getModule(), child); - if (dependency1 && *dependency1 && - EffectAnalyzer(getPassOptions(), *getModule(), *dependency1) - .invalidates(childEffects)) { - return outer; - } - if (dependency2 && *dependency2 && - EffectAnalyzer(getPassOptions(), *getModule(), *dependency2) - .invalidates(childEffects)) { - return outer; - } - } if (auto* block = child->dynCast()) { if (!block->name.is() && block->list.size() >= 2) { auto* back = block->list.back(); @@ -665,7 +647,7 @@ struct MergeBlocks EffectAnalyzer blockChildEffects( getPassOptions(), *getModule(), blockChild); for (auto& effects : childEffects) { - if (blockChildEffects.invalidates(effects)) { + if (effects.orderedBefore(blockChildEffects)) { fail = true; break; } diff --git a/test/lit/passes/merge-blocks-atomics.wast b/test/lit/passes/merge-blocks-atomics.wast new file mode 100644 index 00000000000..cd5ce550644 --- /dev/null +++ b/test/lit/passes/merge-blocks-atomics.wast @@ -0,0 +1,91 @@ +;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited. +;; RUN: wasm-opt %s --merge-blocks -all -S -o - | filecheck %s + +(module + ;; CHECK: (type $struct (shared (struct (field (mut i32))))) + (type $struct (shared (struct (field (mut i32))))) + + ;; CHECK: (memory $mem 1 1 shared) + (memory $mem 1 1 shared) + + ;; CHECK: (func $foo (type $2) (param $0 i32) (param $1 i32) + ;; CHECK-NEXT: ) + (func $foo (param i32 i32)) + + ;; Test 1: Disallowed reordering (GC read NOT moved before Wasm acquire load) + ;; Child 0 is named, so it is NOT optimized and its effects (acquire load) are left behind. + ;; Child 1 tries to move the GC read out, which should be blocked because it cannot move before the acquire load. + ;; CHECK: (func $disallowed (type $1) (param $x (ref $struct)) + ;; CHECK-NEXT: (call $foo + ;; CHECK-NEXT: (block $label1 (result i32) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.atomic.load acqrel + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (block (result i32) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (struct.get $struct 0 + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $disallowed (param $x (ref $struct)) + (call $foo + ;; Child 0: Left behind (named block) + (block $label1 (result i32) + (drop (i32.atomic.load acqrel (i32.const 0))) + (i32.const 0) + ) + ;; Child 1: Tries to move out + (block (result i32) + ;; A: GC read + (drop (struct.get $struct 0 (local.get $x))) + ;; B + (i32.const 0) + ) + ) + ) + + ;; Test 2: Allowed reordering (GC read moved before Wasm release store) + ;; Child 0 is named, so it is NOT optimized and its effects (release store) are left behind. + ;; Child 1 tries to move the GC read out, which should be ALLOWED because it can move before the release store. + ;; CHECK: (func $allowed (type $1) (param $x (ref $struct)) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (struct.get $struct 0 + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (call $foo + ;; CHECK-NEXT: (block $label2 (result i32) + ;; CHECK-NEXT: (i32.atomic.store acqrel + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (i32.const 42) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $allowed (param $x (ref $struct)) + (call $foo + ;; Child 0: Left behind (named block) + (block $label2 (result i32) + (i32.atomic.store acqrel (i32.const 0) (i32.const 42)) + (i32.const 0) + ) + ;; Child 1: Tries to move out + (block (result i32) + ;; A: GC read + (drop (struct.get $struct 0 (local.get $x))) + ;; B + (i32.const 0) + ) + ) + ) +)