Re-land #2235 with fixes (#2245)

#2242 had exposed the bug that the `Trapper` pass was defining `walkFunction` when it should have been defining `doWalkFunction`.

Author: tlively

src/ir/manipulation.h

@@ -38,6 +38,11 @@ template<typename InputType> inline Nop* nop(InputType* target) {
   return convert<InputType, Nop>(target);
 }
 
+template<typename InputType>
+inline Unreachable* unreachable(InputType* target) {
+  return convert<InputType, Unreachable>(target);
+}
+
 // Convert a node that allocates
 template<typename InputType, typename OutputType>
 inline OutputType* convert(InputType* input, MixedArena& allocator) {

src/passes/MemoryPacking.cpp

@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+#include "ir/manipulation.h"
+#include "ir/utils.h"
 #include "pass.h"
 #include "wasm-binary.h"
 #include "wasm-builder.h"
@@ -32,11 +34,13 @@ struct MemoryPacking : public Pass {
       return;
     }
 
-    // Conservatively refuse to change segments if any are passive to avoid
-    // invalidating segment indices or segment contents referenced from
-    // memory.init instructions.
-    // TODO: optimize in the presence of memory.init instructions
     if (module->features.hasBulkMemory()) {
+      // Remove any references to active segments that might be invalidated.
+      optimizeTrappingBulkMemoryOps(runner, module);
+      // Conservatively refuse to change segments if any are passive to avoid
+      // invalidating segment indices or segment contents referenced from
+      // memory.init and data.drop instructions.
+      // TODO: optimize in the presence of memory.init and  data.drop
       for (auto segment : module->memory.segments) {
         if (segment.isPassive) {
           return;
@@ -120,6 +124,40 @@ struct MemoryPacking : public Pass {
     }
     module->memory.segments.swap(packed);
   }
+
+  void optimizeTrappingBulkMemoryOps(PassRunner* runner, Module* module) {
+    struct Trapper : WalkerPass<PostWalker<Trapper>> {
+      bool isFunctionParallel() override { return true; }
+      bool changed;
+
+      Pass* create() override { return new Trapper; }
+
+      void visitMemoryInit(MemoryInit* curr) {
+        if (!getModule()->memory.segments[curr->segment].isPassive) {
+          Builder builder(*getModule());
+          replaceCurrent(builder.blockify(builder.makeDrop(curr->dest),
+                                          builder.makeDrop(curr->offset),
+                                          builder.makeDrop(curr->size),
+                                          builder.makeUnreachable()));
+          changed = true;
+        }
+      }
+      void visitDataDrop(DataDrop* curr) {
+        if (!getModule()->memory.segments[curr->segment].isPassive) {
+          ExpressionManipulator::unreachable(curr);
+          changed = true;
+        }
+      }
+      void doWalkFunction(Function* func) {
+        changed = false;
+        super::doWalkFunction(func);
+        if (changed) {
+          ReFinalize().walkFunctionInModule(func, getModule());
+        }
+      }
+    } trapper;
+    trapper.run(runner, module);
+  }
 };
 
 Pass* createMemoryPackingPass() { return new MemoryPacking(); }

src/wasm/wasm-validator.cpp

@@ -970,8 +970,6 @@ void FunctionValidator::visitSIMDShift(SIMDShift* curr) {
 }
 
 void FunctionValidator::visitMemoryInit(MemoryInit* curr) {
-  shouldBeTrue(
-    getModule()->memory.exists, curr, "Memory operations require a memory");
   shouldBeTrue(getModule()->features.hasBulkMemory(),
                curr,
                "Bulk memory operation (bulk memory is disabled)");
@@ -983,27 +981,33 @@ void FunctionValidator::visitMemoryInit(MemoryInit* curr) {
     curr->offset->type, i32, curr, "memory.init offset must be an i32");
   shouldBeEqualOrFirstIsUnreachable(
     curr->size->type, i32, curr, "memory.init size must be an i32");
+  if (!shouldBeTrue(getModule()->memory.exists,
+                    curr,
+                    "Memory operations require a memory")) {
+    return;
+  }
   shouldBeTrue(curr->segment < getModule()->memory.segments.size(),
                curr,
                "memory.init segment index out of bounds");
 }
 
 void FunctionValidator::visitDataDrop(DataDrop* curr) {
-  shouldBeTrue(
-    getModule()->memory.exists, curr, "Memory operations require a memory");
   shouldBeTrue(getModule()->features.hasBulkMemory(),
                curr,
                "Bulk memory operation (bulk memory is disabled)");
   shouldBeEqualOrFirstIsUnreachable(
     curr->type, none, curr, "data.drop must have type none");
+  if (!shouldBeTrue(getModule()->memory.exists,
+                    curr,
+                    "Memory operations require a memory")) {
+    return;
+  }
   shouldBeTrue(curr->segment < getModule()->memory.segments.size(),
                curr,
                "data.drop segment index out of bounds");
 }
 
 void FunctionValidator::visitMemoryCopy(MemoryCopy* curr) {
-  shouldBeTrue(
-    getModule()->memory.exists, curr, "Memory operations require a memory");
   shouldBeTrue(getModule()->features.hasBulkMemory(),
                curr,
                "Bulk memory operation (bulk memory is disabled)");
@@ -1015,11 +1019,11 @@ void FunctionValidator::visitMemoryCopy(MemoryCopy* curr) {
     curr->source->type, i32, curr, "memory.copy source must be an i32");
   shouldBeEqualOrFirstIsUnreachable(
     curr->size->type, i32, curr, "memory.copy size must be an i32");
+  shouldBeTrue(
+    getModule()->memory.exists, curr, "Memory operations require a memory");
 }
 
 void FunctionValidator::visitMemoryFill(MemoryFill* curr) {
-  shouldBeTrue(
-    getModule()->memory.exists, curr, "Memory operations require a memory");
   shouldBeTrue(getModule()->features.hasBulkMemory(),
                curr,
                "Bulk memory operation (bulk memory is disabled)");
@@ -1031,6 +1035,8 @@ void FunctionValidator::visitMemoryFill(MemoryFill* curr) {
     curr->value->type, i32, curr, "memory.fill value must be an i32");
   shouldBeEqualOrFirstIsUnreachable(
     curr->size->type, i32, curr, "memory.fill size must be an i32");
+  shouldBeTrue(
+    getModule()->memory.exists, curr, "Memory operations require a memory");
 }
 
 void FunctionValidator::validateMemBytes(uint8_t bytes,

test/passes/memory-packing.txt renamed to test/passes/memory-packing_all-features.txt

@@ -18,3 +18,30 @@
  (import "env" "memory" (memory $0 2048 2048))
  (import "env" "memoryBase" (global $memoryBase i32))
 )
+(module
+ (type $FUNCSIG$v (func))
+ (memory $0 1 1)
+ (func $foo (; 0 ;) (type $FUNCSIG$v)
+  (block
+   (drop
+    (i32.const 0)
+   )
+   (drop
+    (i32.const 0)
+   )
+   (drop
+    (i32.const 0)
+   )
+   (unreachable)
+  )
+  (unreachable)
+ )
+ (func $bar (; 1 ;) (type $FUNCSIG$v)
+  (drop
+   (loop $loop-in (result i32)
+    (unreachable)
+    (i32.const 42)
+   )
+  )
+ )
+)

test/passes/memory-packing.wast renamed to test/passes/memory-packing_all-features.wast

@@ -17,4 +17,23 @@
   (import "env" "memoryBase" (global $memoryBase i32))
   (data (i32.const 4066) "") ;; empty
 )
-
+(module
+ (memory $0 1 1)
+ (data (i32.const 0) "")
+ (func $foo
+  (memory.init 0
+   (i32.const 0)
+   (i32.const 0)
+   (i32.const 0)
+  )
+  (data.drop 0)
+ )
+ (func $bar
+  (drop
+   (loop (result i32)
+    (data.drop 0)
+    (i32.const 42)
+   )
+  )
+ )
+)