Revert "Remove bulk memory instructions refering to active segments (#2235)" (#2244)

This reverts commit 72c52ea, which was causing test failures after it merged.

Author: tlively

src/ir/manipulation.h

@@ -38,11 +38,6 @@ 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,8 +14,6 @@
  * limitations under the License.
  */
 
-#include "ir/manipulation.h"
-#include "ir/utils.h"
 #include "pass.h"
 #include "wasm-binary.h"
 #include "wasm-builder.h"
@@ -34,13 +32,11 @@ 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;
@@ -124,38 +120,6 @@ struct MemoryPacking : public Pass {
     }
     module->memory.segments.swap(packed);
   }
-
-  void optimizeTrappingBulkMemoryOps(PassRunner* runner, Module* module) {
-    struct Trapper : WalkerPass<PostWalker<Trapper>> {
-      bool changed;
-      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 walkFunction(Function* func) {
-        changed = false;
-        PostWalker<Trapper>::walkFunction(func);
-        if (changed) {
-          ReFinalize().walkFunctionInModule(func, getModule());
-        }
-      }
-      bool isFunctionParallel() override { return true; }
-      Pass* create() override { return new Trapper; }
-    } trapper;
-    trapper.run(runner, module);
-  }
 };
 
 Pass* createMemoryPackingPass() { return new MemoryPacking(); }

src/wasm/wasm-validator.cpp

@@ -970,6 +970,8 @@ 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)");
@@ -981,33 +983,27 @@ 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)");
@@ -1019,11 +1015,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)");
@@ -1035,8 +1031,6 @@ 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,

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

@@ -18,30 +18,3 @@
  (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)
-   )
-  )
- )
-)

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

@@ -17,23 +17,4 @@
   (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)
-   )
-  )
- )
-)
+