Prevent pops from sinking in SimplifyLocals (#2885)

This prevents `exnref.pop`s from being sinked and separated from
`catch`. For example,
```wast
(try
  (do)
  (catch
    (local.set $0 (exnref.pop))
    (call $foo
      (i32.const 3)
      (local.get $0)
    )
  )
)
```
Here, if we sink `exnref.pop` to remove `local.set $0` and
`local.get $0`, it becomes this:
```wast
(try
  (do)
  (catch
    (nop)
    (call $foo
      (i32.const 3)
      (exnref.pop)
    )
  )
)
```
This move was possible because `i32.const 3` does not have any side
effects. But this is incorrect because now `exnref.pop` does not follow
right after `catch`.

To prevent this, this patch checks this case in `canSink` in
SimplifyLocals. When we encountered a similar case in CodeFolding, we
prevented every expression that contains `Pop` anywhere in it from being
moved, which was too conservative. This adds `danglingPop` property in
`EffectAnalyzer`, so that only pops that are not enclosed within a
`catch` count as 'dangling pops` and we only prevent those pops from
being moved or sinked.

Author: aheejin

src/binaryen-c.cpp

@@ -2898,6 +2898,10 @@ BinaryenSideEffects BinaryenSideEffectIsAtomic(void) {
 BinaryenSideEffects BinaryenSideEffectThrows(void) {
   return static_cast<BinaryenSideEffects>(EffectAnalyzer::SideEffects::Throws);
 }
+BinaryenSideEffects BinaryenSideEffectDanglingPop(void) {
+  return static_cast<BinaryenSideEffects>(
+    EffectAnalyzer::SideEffects::DanglingPop);
+}
 BinaryenSideEffects BinaryenSideEffectAny(void) {
   return static_cast<BinaryenSideEffects>(EffectAnalyzer::SideEffects::Any);
 }

src/binaryen-c.h

@@ -1580,6 +1580,7 @@ BINARYEN_API BinaryenSideEffects BinaryenSideEffectWritesMemory(void);
 BINARYEN_API BinaryenSideEffects BinaryenSideEffectImplicitTrap(void);
 BINARYEN_API BinaryenSideEffects BinaryenSideEffectIsAtomic(void);
 BINARYEN_API BinaryenSideEffects BinaryenSideEffectThrows(void);
+BINARYEN_API BinaryenSideEffects BinaryenSideEffectDanglingPop(void);
 BINARYEN_API BinaryenSideEffects BinaryenSideEffectAny(void);
 
 BINARYEN_API BinaryenSideEffects BinaryenExpressionGetSideEffects(

src/ir/effects.h

@@ -76,13 +76,19 @@ struct EffectAnalyzer
   // inner try, we don't mark it as 'throws', because it will be caught by an
   // inner catch.
   size_t tryDepth = 0;
+  // The nested depth of catch. This is necessary to track danglng pops.
+  size_t catchDepth = 0;
+  // If this expression contains 'exnref.pop's that are not enclosed in 'catch'
+  // body. For example, (drop (exnref.pop)) should set this to true.
+  bool danglingPop = false;
 
   static void scan(EffectAnalyzer* self, Expression** currp) {
     Expression* curr = *currp;
     // We need to decrement try depth before catch starts, so handle it
     // separately
     if (curr->is<Try>()) {
       self->pushTask(doVisitTry, currp);
+      self->pushTask(doEndCatch, currp);
       self->pushTask(scan, &curr->cast<Try>()->catchBody);
       self->pushTask(doStartCatch, currp);
       self->pushTask(scan, &curr->cast<Try>()->body);
@@ -100,6 +106,12 @@ struct EffectAnalyzer
   static void doStartCatch(EffectAnalyzer* self, Expression** currp) {
     assert(self->tryDepth > 0 && "try depth cannot be negative");
     self->tryDepth--;
+    self->catchDepth++;
+  }
+
+  static void doEndCatch(EffectAnalyzer* self, Expression** currp) {
+    assert(self->catchDepth > 0 && "catch depth cannot be negative");
+    self->catchDepth--;
   }
 
   // Helper functions to check for various effect types
@@ -128,7 +140,7 @@ struct EffectAnalyzer
   }
   bool hasSideEffects() const {
     return hasGlobalSideEffects() || localsWritten.size() > 0 ||
-           transfersControlFlow() || implicitTrap;
+           transfersControlFlow() || implicitTrap || danglingPop;
   }
   bool hasAnything() const {
     return hasSideEffects() || accessesLocal() || readsMemory ||
@@ -148,7 +160,8 @@ struct EffectAnalyzer
     if ((transfersControlFlow() && other.hasSideEffects()) ||
         (other.transfersControlFlow() && hasSideEffects()) ||
         ((writesMemory || calls) && other.accessesMemory()) ||
-        (accessesMemory() && (other.writesMemory || other.calls))) {
+        (accessesMemory() && (other.writesMemory || other.calls)) ||
+        (danglingPop || other.danglingPop)) {
       return true;
     }
     // All atomics are sequentially consistent for now, and ordered wrt other
@@ -203,6 +216,7 @@ struct EffectAnalyzer
     implicitTrap = implicitTrap || other.implicitTrap;
     isAtomic = isAtomic || other.isAtomic;
     throws = throws || other.throws;
+    danglingPop = danglingPop || other.danglingPop;
     for (auto i : other.localsRead) {
       localsRead.insert(i);
     }
@@ -470,7 +484,11 @@ struct EffectAnalyzer
   }
   void visitNop(Nop* curr) {}
   void visitUnreachable(Unreachable* curr) { branchesOut = true; }
-  void visitPop(Pop* curr) { calls = true; }
+  void visitPop(Pop* curr) {
+    if (catchDepth == 0) {
+      danglingPop = true;
+    }
+  }
   void visitTupleMake(TupleMake* curr) {}
   void visitTupleExtract(TupleExtract* curr) {}
 
@@ -500,7 +518,8 @@ struct EffectAnalyzer
     ImplicitTrap = 1 << 8,
     IsAtomic = 1 << 9,
     Throws = 1 << 10,
-    Any = (1 << 11) - 1
+    DanglingPop = 1 << 11,
+    Any = (1 << 12) - 1
   };
   uint32_t getSideEffects() const {
     uint32_t effects = 0;
@@ -537,6 +556,9 @@ struct EffectAnalyzer
     if (throws) {
       effects |= SideEffects::Throws;
     }
+    if (danglingPop) {
+      effects |= SideEffects::DanglingPop;
+    }
     return effects;
   }
 

src/js/binaryen.js-post.js

@@ -485,6 +485,7 @@ function initializeConstants() {
     'ImplicitTrap',
     'IsAtomic',
     'Throws',
+    'DanglingPop',
     'Any'
   ].forEach(function(name) {
     Module['SideEffects'][name] = Module['_BinaryenSideEffect' + name]();

src/passes/CodeFolding.cpp

@@ -305,12 +305,12 @@ struct CodeFolding : public WalkerPass<ControlFlowWalker<CodeFolding>> {
         return false;
       }
       if (getModule()->features.hasExceptionHandling()) {
+        EffectAnalyzer effects(getPassOptions(), getModule()->features, item);
         // Currently pop instructions are only used for exnref.pop, which is a
-        // pseudo instruction following a catch. We check if the current
-        // expression has a pop child. This can be overly conservative, because
-        // this can also exclude whole try-catches that contain a pop within
-        // them.
-        if (!FindAll<Pop>(item).list.empty()) {
+        // pseudo instruction following a catch. We cannot move expressions
+        // containing pops if they are not enclosed in a 'catch' body, because a
+        // pop instruction should follow right after 'catch'.
+        if (effects.danglingPop) {
           return false;
         }
         // When an expression can throw and it is within a try scope, taking it
@@ -321,9 +321,7 @@ struct CodeFolding : public WalkerPass<ControlFlowWalker<CodeFolding>> {
         // conservative approximation because there can be cases that 'try' is
         // within the expression that may throw so it is safe to take the
         // expression out.
-        if (EffectAnalyzer(getPassOptions(), getModule()->features, item)
-              .throws &&
-            !FindAll<Try>(outOf).list.empty()) {
+        if (effects.throws && !FindAll<Try>(outOf).list.empty()) {
           return false;
         }
       }

src/passes/SimplifyLocals.cpp

@@ -411,6 +411,14 @@ struct SimplifyLocals
     if (set->isTee()) {
       return false;
     }
+    // We cannot move expressions containing exnref.pops that are not enclosed
+    // in 'catch', because 'exnref.pop' should follow right after 'catch'.
+    FeatureSet features = this->getModule()->features;
+    if (features.hasExceptionHandling() &&
+        EffectAnalyzer(this->getPassOptions(), features, set->value)
+          .danglingPop) {
+      return false;
+    }
     // if in the first cycle, or not allowing tees, then we cannot sink if >1
     // use as that would make a tee
     if ((firstCycle || !allowTee) && getCounter.num[set->index] > 1) {

test/binaryen.js/sideffects.js

@@ -10,6 +10,7 @@ console.log("SideEffects.WritesMemory=" + binaryen.SideEffects.WritesMemory);
 console.log("SideEffects.ImplicitTrap=" + binaryen.SideEffects.ImplicitTrap);
 console.log("SideEffects.IsAtomic=" + binaryen.SideEffects.IsAtomic);
 console.log("SideEffects.Throws=" + binaryen.SideEffects.Throws);
+console.log("SideEffects.DanglingPop=" + binaryen.SideEffects.DanglingPop);
 console.log("SideEffects.Any=" + binaryen.SideEffects.Any);
 
 var module = new binaryen.Module();
@@ -104,3 +105,12 @@ assert(
   ==
   binaryen.SideEffects.Calls | binaryen.SideEffects.Throws
 );
+
+assert(
+  binaryen.getSideEffects(
+    module.drop(module.exnref.pop()),
+    module.getFeatures()
+  )
+  ==
+  binaryen.SideEffects.DanglingPop
+);

test/binaryen.js/sideffects.js.txt

@@ -10,4 +10,5 @@ SideEffects.WritesMemory=128
 SideEffects.ImplicitTrap=256
 SideEffects.IsAtomic=512
 SideEffects.Throws=1024
-SideEffects.Any=2047
+SideEffects.DanglingPop=2048
+SideEffects.Any=4095

test/passes/simplify-locals_all-features.txt

@@ -1894,6 +1894,7 @@
 )
 (module
  (type $none_=>_none (func))
+ (type $i32_exnref_=>_none (func (param i32 exnref)))
  (type $exnref_=>_none (func (param exnref)))
  (type $none_=>_exnref (func (result exnref)))
  (event $event$0 (attr 0) (param))
@@ -1937,4 +1938,46 @@
    )
   )
  )
+ (func $foo (param $0 i32) (param $1 exnref)
+  (nop)
+ )
+ (func $pop-cannot-be-sinked
+  (local $0 exnref)
+  (try
+   (do
+    (nop)
+   )
+   (catch
+    (local.set $0
+     (exnref.pop)
+    )
+    (call $foo
+     (i32.const 3)
+     (local.get $0)
+    )
+   )
+  )
+ )
+ (func $pop-within-catch-can-be-sinked
+  (local $0 exnref)
+  (try
+   (do
+    (nop)
+   )
+   (catch
+    (nop)
+    (call $foo
+     (i32.const 3)
+     (try (result exnref)
+      (do
+       (ref.null)
+      )
+      (catch
+       (exnref.pop)
+      )
+     )
+    )
+   )
+  )
+ )
 )

test/passes/simplify-locals_all-features.wast

@@ -1711,4 +1711,42 @@
       )
     )
   )
+
+  (func $foo (param i32 exnref))
+  (func $pop-cannot-be-sinked (local $0 exnref)
+    (try
+      (do)
+      (catch
+        ;; This (local.set $0) of (exnref.pop) cannot be sinked to
+        ;; (local.get $0) below, because exnref.pop should follow right after
+        ;; 'catch'.
+        (local.set $0 (exnref.pop))
+        (call $foo
+          (i32.const 3)
+          (local.get $0)
+        )
+      )
+    )
+  )
+
+  (func $pop-within-catch-can-be-sinked (local $0 exnref)
+    (try
+      (do)
+      (catch
+        ;; This whole 'try' body can be sinked to eliminate local.set /
+        ;; local.get. Even though it contains a pop, it is enclosed within
+        ;; try-catch, so it is OK.
+        (local.set $0
+          (try (result exnref)
+            (do (ref.null))
+            (catch (exnref.pop))
+          )
+        )
+        (call $foo
+          (i32.const 3)
+          (local.get $0)
+        )
+      )
+    )
+  )
 )