do not swap elements in conditionalizeExpensiveOnBitwise if they invalidate each other - it is not enough to check side effects, we must check the interaction as well

Author: kripken

src/passes/OptimizeInstructions.cpp

@@ -874,14 +874,17 @@ struct OptimizeInstructions : public WalkerPass<PostWalker<OptimizeInstructions,
     auto* left = binary->left;
     auto* right = binary->right;
     if (!Properties::emitsBoolean(left) || !Properties::emitsBoolean(right)) return nullptr;
-    auto leftEffects = EffectAnalyzer(getPassOptions(), left).hasSideEffects();
-    auto rightEffects = EffectAnalyzer(getPassOptions(), right).hasSideEffects();
-    if (leftEffects && rightEffects) return nullptr; // both must execute
-   // canonicalize with side effects, if any, happening on the left
-    if (rightEffects) {
+    auto leftEffects = EffectAnalyzer(getPassOptions(), left);
+    auto rightEffects = EffectAnalyzer(getPassOptions(), right);
+    auto leftSideEffects = leftEffects.hasSideEffects();
+    auto rightSideEffects = rightEffects.hasSideEffects();
+    if (leftSideEffects && rightSideEffects) return nullptr; // both must execute
+    // canonicalize with side effects, if any, happening on the left
+    if (rightSideEffects) {
       if (CostAnalyzer(left).cost < MIN_COST) return nullptr; // avoidable code is too cheap
+      if (leftEffects.invalidates(rightEffects)) return nullptr; // cannot reorder
       std::swap(left, right);
-    } else if (leftEffects) {
+    } else if (leftSideEffects) {
       if (CostAnalyzer(right).cost < MIN_COST) return nullptr; // avoidable code is too cheap
     } else {
       // no side effects, reorder based on cost estimation

test/passes/optimize-instructions_optimize-level=2_ignore-implicit-traps.txt

@@ -251,4 +251,74 @@
    (get_local $5)
   )
  )
+ (func $invalidate-conditionalizeExpensiveOnBitwise (type $0) (param $0 i32) (param $1 i32) (result i32)
+  (if
+   (i32.eqz
+    (i32.and
+     (i32.lt_s
+      (i32.and
+       (i32.shr_s
+        (i32.shl
+         (i32.add
+          (get_local $1)
+          (i32.const -1)
+         )
+         (i32.const 24)
+        )
+        (i32.const 24)
+       )
+       (i32.const 255)
+      )
+      (i32.const 3)
+     )
+     (i32.ne
+      (tee_local $1
+       (i32.const 0)
+      )
+      (i32.const 0)
+     )
+    )
+   )
+   (return
+    (get_local $0)
+   )
+  )
+  (return
+   (get_local $1)
+  )
+ )
+ (func $invalidate-conditionalizeExpensiveOnBitwise-ok (type $0) (param $0 i32) (param $1 i32) (result i32)
+  (if
+   (i32.eqz
+    (if (result i32)
+     (tee_local $1
+      (i32.const 0)
+     )
+     (i32.lt_s
+      (i32.and
+       (i32.shr_s
+        (i32.shl
+         (i32.add
+          (get_local $0)
+          (i32.const -1)
+         )
+         (i32.const 24)
+        )
+        (i32.const 24)
+       )
+       (i32.const 255)
+      )
+      (i32.const 3)
+     )
+     (i32.const 0)
+    )
+   )
+   (return
+    (get_local $0)
+   )
+  )
+  (return
+   (get_local $1)
+  )
+ )
 )

test/passes/optimize-instructions_optimize-level=2_ignore-implicit-traps.wast

@@ -259,5 +259,69 @@
       (get_local $5)
     )
   )
+  (func $invalidate-conditionalizeExpensiveOnBitwise (param $0 i32) (param $1 i32) (result i32)
+   (if
+    (i32.eqz
+     (i32.and
+      (i32.lt_s
+       (i32.and
+        (i32.shr_s
+         (i32.shl
+          (i32.add
+           (get_local $1) ;; conflict with tee
+           (i32.const -1)
+          )
+          (i32.const 24)
+         )
+         (i32.const 24)
+        )
+        (i32.const 255)
+       )
+       (i32.const 3)
+      )
+      (i32.ne
+       (tee_local $1
+        (i32.const 0)
+       )
+       (i32.const 0)
+      )
+     )
+    )
+    (return (get_local $0))
+   )
+   (return (get_local $1))
+  )
+  (func $invalidate-conditionalizeExpensiveOnBitwise-ok (param $0 i32) (param $1 i32) (result i32)
+   (if
+    (i32.eqz
+     (i32.and
+      (i32.lt_s
+       (i32.and
+        (i32.shr_s
+         (i32.shl
+          (i32.add
+           (get_local $0) ;; no conflict
+           (i32.const -1)
+          )
+          (i32.const 24)
+         )
+         (i32.const 24)
+        )
+        (i32.const 255)
+       )
+       (i32.const 3)
+      )
+      (i32.ne
+       (tee_local $1
+        (i32.const 0)
+       )
+       (i32.const 0)
+      )
+     )
+    )
+    (return (get_local $0))
+   )
+   (return (get_local $1))
+  )
 )