diff --git a/src/ir/effects.h b/src/ir/effects.h index 44cc8031f45..666765364f7 100644 --- a/src/ir/effects.h +++ b/src/ir/effects.h @@ -303,10 +303,8 @@ class EffectAnalyzer { // (e.g., if we write, we must remain ordered before someone that reads). // // This assumes the things whose effects we are comparing will both execute, - // at least if neither of them transfers control flow away. That is, we assume - // that there is no transfer of control flow *between* them: we are comparing - // things appear after each other, perhaps with some other code in the middle, - // but that code does not transfer control flow. It is not valid to call this + // at least if neither of them transfers control flow away. We assume there is + // no transfer of control flow *between* them. It is not valid to call this // method in other situations, like this: // // A @@ -326,12 +324,15 @@ class EffectAnalyzer { // ;; control flow transfer // B // - // That the things being compared both execute only matters in the case of - // traps-never-happen: in that mode we can move traps but only if doing so - // would not make them start to appear when they did not. In the second - // example we can't reorder A and B if B traps, but in the first example we - // can reorder them even if B traps (even if A has a global effect like a - // global.set, since we assume B does not trap in traps-never-happen). + // (Note that if they appear in inside a loop, A and B may overlap or even be + // the same expression; this is fine because A still executes before B, even + // if also executes during and after B across different loop iterations.) That + // A and B both execute only matters in the case of traps-never-happen: in + // that mode we can move traps but only if doing so would not make them start + // to appear when they did not previously. In the example with the br_if we + // can't reorder A and B if B traps, but in the valid examples we can reorder + // them even if B traps (even if A has a global effect like a global.set, + // since we assume B does not trap in traps-never-happen). bool orderedBefore(const EffectAnalyzer& other) const { // Cannot reorder control flow and side effects. if ((transfersControlFlow() && other.hasSideEffects()) || diff --git a/src/passes/LoopInvariantCodeMotion.cpp b/src/passes/LoopInvariantCodeMotion.cpp index 6add5134a79..c524b82ae9d 100644 --- a/src/passes/LoopInvariantCodeMotion.cpp +++ b/src/passes/LoopInvariantCodeMotion.cpp @@ -65,11 +65,14 @@ struct LoopInvariantCodeMotion // is ok to do so. EffectAnalyzer effectsSoFar(getPassOptions(), *getModule()); // The loop's total effects also matter. For example, a store - // in the loop means we can't move a load outside. + // in the loop means we can't move a load outside. We discard the local + // reads and writes because we analyze them separately. // FIXME: we look at the loop "tail" area too, after the last // possible branch back, which can cause false positives // for bad effect interactions. EffectAnalyzer loopEffects(getPassOptions(), *getModule(), loop); + loopEffects.localsRead.clear(); + loopEffects.localsWritten.clear(); // Note all the sets in each loop, and how many per index. Currently // EffectAnalyzer can't do that, and we need it to know if we // can move a set out of the loop (if there is another set @@ -123,9 +126,8 @@ struct LoopInvariantCodeMotion // take into account global state like interacting loads and // stores. bool unsafeToMove = effects.writesGlobalState() || - effectsSoFar.invalidates(effects) || - (effects.readsMutableGlobalState() && - loopEffects.writesGlobalState()); + effectsSoFar.orderedBefore(effects) || + loopEffects.orderedBefore(effects); // TODO: look into optimizing this with exceptions. for now, disallow if (effects.throws() || loopEffects.throws()) { unsafeToMove = true; diff --git a/test/lit/passes/licm-atomics.wast b/test/lit/passes/licm-atomics.wast new file mode 100644 index 00000000000..43beb342a0c --- /dev/null +++ b/test/lit/passes/licm-atomics.wast @@ -0,0 +1,68 @@ +;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited. +;; RUN: foreach %s %t wasm-opt -all --licm -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) + + ;; Test 1: Allowed reordering (GC read moved before Wasm 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: (loop $loop + ;; CHECK-NEXT: (i32.atomic.store acqrel + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (i32.const 42) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: (br $loop) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $allowed (param $x (ref $struct)) + (loop $loop + ;; X: release store (Wasm memory) + (i32.atomic.store acqrel (i32.const 0) (i32.const 42)) + ;; E: memory access (shared GC read) + (drop + (struct.get $struct 0 (local.get $x)) + ) + (br $loop) + ) + ) + + ;; Test 2: Disallowed reordering (GC read moved before Wasm acquire load) + ;; CHECK: (func $disallowed (type $1) (param $x (ref $struct)) + ;; CHECK-NEXT: (loop $loop + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.atomic.load acqrel + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (struct.get $struct 0 + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (br $loop) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $disallowed (param $x (ref $struct)) + (loop $loop + ;; X: acquire load (Wasm memory) + (drop + (i32.atomic.load acqrel (i32.const 0)) + ) + ;; E: memory access (shared GC read) + (drop + (struct.get $struct 0 (local.get $x)) + ) + (br $loop) + ) + ) +) diff --git a/test/lit/passes/licm.wast b/test/lit/passes/licm.wast index 0d82ad4a943..d1918c95491 100644 --- a/test/lit/passes/licm.wast +++ b/test/lit/passes/licm.wast @@ -7,11 +7,13 @@ ;; CHECK: (type $0 (func (param i32))) - ;; CHECK: (type $1 (func)) + ;; CHECK: (type $1 (func (param i32) (result i32))) + + ;; CHECK: (type $2 (func)) ;; CHECK: (memory $0 10 20) - ;; CHECK: (func $unreachable-get (type $1) + ;; CHECK: (func $unreachable-get (type $2) ;; CHECK-NEXT: (local $x i32) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (local.get $x) @@ -104,4 +106,80 @@ (br $loop) ) ) + + ;; CHECK: (func $bug-inversion (type $1) (param $z i32) (result i32) + ;; CHECK-NEXT: (local $x i32) + ;; CHECK-NEXT: (local $y i32) + ;; CHECK-NEXT: (local.set $y + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.set $x + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (loop $loop + ;; CHECK-NEXT: (local.set $y + ;; CHECK-NEXT: (i32.const 2) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.set $x + ;; CHECK-NEXT: (i32.add + ;; CHECK-NEXT: (local.get $y) + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (br_if $loop + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + (func $bug-inversion (param $z i32) (result i32) + (local $x i32) + (local $y i32) + (local.set $y (i32.const 0)) + (local.set $x (i32.const 0)) + (loop $loop + (local.set $y (i32.const 2)) + (local.set $x (i32.add (local.get $y) (i32.const 1))) + (br_if $loop (i32.const 0)) + ) + (local.get $x) + ) + + ;; CHECK: (func $bug-cross-statement-dependency (type $1) (param $z i32) (result i32) + ;; CHECK-NEXT: (local $x i32) + ;; CHECK-NEXT: (local $y i32) + ;; CHECK-NEXT: (local.set $x + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.set $y + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (loop $loop + ;; CHECK-NEXT: (local.set $y + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.set $x + ;; CHECK-NEXT: (i32.add + ;; CHECK-NEXT: (local.get $z) + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (br_if $loop + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.get $y) + ;; CHECK-NEXT: ) + (func $bug-cross-statement-dependency (param $z i32) (result i32) + (local $x i32) + (local $y i32) + (local.set $x (i32.const 0)) + (local.set $y (i32.const 0)) + (loop $loop + (local.set $y (local.get $x)) + (local.set $x (i32.add (local.get $z) (i32.const 1))) + (br_if $loop (i32.const 0)) + ) + (local.get $y) + ) )