From 072b3b79bd7fb5ec5cedffe5f62da73c07bc4808 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Fri, 5 Jun 2026 14:54:56 -0700 Subject: [PATCH 1/2] Use deep effects in areConsecutiveInputsEqual Although it is true that side effects in the children of the LHS input affect both the LHS and RHS inputs equally, we did not properly account for the fact that the same effects are executed again as part of the RHS and can change its value. --- src/passes/OptimizeInstructions.cpp | 32 +++++++----- ...nstructions-global-effects-idempotent.wast | 39 +++++++++++++- .../lit/passes/optimize-instructions-mvp.wast | 52 +++++++++++++++++++ 3 files changed, 108 insertions(+), 15 deletions(-) diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp index 6cce997c2bf..dff4c0ecc8a 100644 --- a/src/passes/OptimizeInstructions.cpp +++ b/src/passes/OptimizeInstructions.cpp @@ -2873,24 +2873,28 @@ struct OptimizeInstructions return false; } - // They do look the same! Determine whether the left hand expression can - // change the value of the operands flowing into the right-hand expression. - // Do not consider the right-hand expression itself here because the - // expressions might be idempotent function calls and we might otherwise - // mistakenly conclude that the first call interferes with the second. - EffectAnalyzer rightEffects(getPassOptions(), *getModule()); + // They do look the same! Determine whether there are side effects that + // could make the values different. + EffectAnalyzer fallthroughChildEffects(getPassOptions(), *getModule()); for (auto* child : ChildIterator(right)) { - rightEffects.walk(child); - } - ShallowEffectAnalyzer leftEffects(getPassOptions(), *getModule(), left); - if (leftEffects.orderedBefore(rightEffects)) { + fallthroughChildEffects.walk(child); + } + EffectAnalyzer fallthroughEffects = fallthroughChildEffects; + fallthroughEffects.visit(right); + + // Does the left-hand parent expression or side effects in the right-hand + // children affect the values flowing into the right-hand parent expression? + // Do not consider the right-hand parent expression because it could be an + // idempotent call and we don't want to mistakenly conclude that the + // left-hand side could interfere with it. (Non-idempotent functions will + // be rejected by the `isGenerative` check below.) + if (fallthroughEffects.orderedBefore(fallthroughChildEffects)) { return false; } - // Make sure nothing executed in between the expressions can affect the - // value of `right` (or its operands) and make it different from `left`. - rightEffects.visit(right); - if (interferingEffects.orderedBefore(rightEffects)) { + // Do any effects executed between the expressions affect the right-hand + // expression? + if (interferingEffects.orderedBefore(fallthroughEffects)) { return false; } diff --git a/test/lit/passes/optimize-instructions-global-effects-idempotent.wast b/test/lit/passes/optimize-instructions-global-effects-idempotent.wast index dbb6af3da83..8ee7c6f175c 100644 --- a/test/lit/passes/optimize-instructions-global-effects-idempotent.wast +++ b/test/lit/passes/optimize-instructions-global-effects-idempotent.wast @@ -1,5 +1,6 @@ ;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited. -;; RUN: wasm-opt --generate-global-effects --optimize-instructions -all -S -o - %s | filecheck %s +;; RUN: foreach %s %t wasm-opt --generate-global-effects --optimize-instructions -all -S -o - \ +;; RUN: | filecheck %s (module ;; CHECK: (type $struct (shared (struct (field (mut i32))))) @@ -91,3 +92,39 @@ ) ) ) + +(module + ;; CHECK: (global $f (mut f32) (f32.const -3)) + (global $f (mut f32) (f32.const -3)) + + ;; CHECK: (func $potent (type $0) (result f32) + ;; CHECK-NEXT: (global.set $f + ;; CHECK-NEXT: (f32.add + ;; CHECK-NEXT: (global.get $f) + ;; CHECK-NEXT: (f32.const 2) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (global.get $f) + ;; CHECK-NEXT: ) + (func $potent (result f32) + (global.set $f (f32.add (global.get $f) (f32.const 2))) + (global.get $f) + ) + + ;; CHECK: (func $function-effects-interfere (type $0) (result f32) + ;; CHECK-NEXT: (f32.abs + ;; CHECK-NEXT: (f32.mul + ;; CHECK-NEXT: (call $potent) + ;; CHECK-NEXT: (call $potent) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $function-effects-interfere (result f32) + (f32.abs + (f32.mul + (call $potent) + (call $potent) + ) + ) + ) +) diff --git a/test/lit/passes/optimize-instructions-mvp.wast b/test/lit/passes/optimize-instructions-mvp.wast index 021b868bcc1..3f255703b9a 100644 --- a/test/lit/passes/optimize-instructions-mvp.wast +++ b/test/lit/passes/optimize-instructions-mvp.wast @@ -13,6 +13,9 @@ (memory 0) + ;; CHECK: (global $g-f32 (mut f32) (f32.const -3)) + (global $g-f32 (mut f32) (f32.const -3)) + ;; CHECK: (func $and-and (param $i1 i32) (result i32) ;; CHECK-NEXT: (i32.and ;; CHECK-NEXT: (local.get $i1) @@ -15757,6 +15760,55 @@ ) )) ) + ;; CHECK: (func $optimize-float-points-stateful (result f32) + ;; CHECK-NEXT: (f32.abs + ;; CHECK-NEXT: (f32.mul + ;; CHECK-NEXT: (f32.neg + ;; CHECK-NEXT: (block (result f32) + ;; CHECK-NEXT: (global.set $g-f32 + ;; CHECK-NEXT: (f32.add + ;; CHECK-NEXT: (global.get $g-f32) + ;; CHECK-NEXT: (f32.const 2) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (global.get $g-f32) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (f32.neg + ;; CHECK-NEXT: (block (result f32) + ;; CHECK-NEXT: (global.set $g-f32 + ;; CHECK-NEXT: (f32.add + ;; CHECK-NEXT: (global.get $g-f32) + ;; CHECK-NEXT: (f32.const 2) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (global.get $g-f32) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $optimize-float-points-stateful (result f32) + (f32.abs + (f32.mul + ;; LHS + (f32.neg + (block (result f32) + (global.set $g-f32 (f32.add (global.get $g-f32) (f32.const 2))) + (global.get $g-f32) + ) + ) + ;; RHS - The increment of the global causes this to produce a different + ;; result than the LHS, so we cannot optimize out the f32.abs. + (f32.neg + (block (result f32) + (global.set $g-f32 (f32.add (global.get $g-f32) (f32.const 2))) + (global.get $g-f32) + ) + ) + ) + ) + ) ;; CHECK: (func $ternary (param $x i32) (param $y i32) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.eqz From 0140edf8800cc3c4323a6798b641ce3f03954ad3 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Fri, 5 Jun 2026 17:25:17 -0700 Subject: [PATCH 2/2] more comments --- src/passes/OptimizeInstructions.cpp | 4 +++- .../optimize-instructions-global-effects-idempotent.wast | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp index dff4c0ecc8a..ba9989b330e 100644 --- a/src/passes/OptimizeInstructions.cpp +++ b/src/passes/OptimizeInstructions.cpp @@ -2874,7 +2874,9 @@ struct OptimizeInstructions } // They do look the same! Determine whether there are side effects that - // could make the values different. + // could make the values different. Note that we use the same + // EffectAnalyzers for effects from the left and right sides because they + // are the same. EffectAnalyzer fallthroughChildEffects(getPassOptions(), *getModule()); for (auto* child : ChildIterator(right)) { fallthroughChildEffects.walk(child); diff --git a/test/lit/passes/optimize-instructions-global-effects-idempotent.wast b/test/lit/passes/optimize-instructions-global-effects-idempotent.wast index 8ee7c6f175c..8c5365260a9 100644 --- a/test/lit/passes/optimize-instructions-global-effects-idempotent.wast +++ b/test/lit/passes/optimize-instructions-global-effects-idempotent.wast @@ -120,6 +120,8 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) (func $function-effects-interfere (result f32) + ;; Even knowing the effects of the call, we should not be able to optimize + ;; here because the calls will return different values. (f32.abs (f32.mul (call $potent)