wasm-merge: Avoid quadratic time in validation [NFC] (#7845)

The actual merging logic in wasm-merge is careful to run in linear time
(since #5709). However, validation could actually be quadratic, since we do
it after each module that we merge in. Since we validate by default, this
made wasm-merge very slow sometimes.

To fix that, validate by default once at the very end. When in pass-debug
mode, which validates after each pass, validate after each module (so we
keep the option for precise debugging of problems).

Also, move some validation code being the validation flag, so that
--no-validation is even faster.

On a large testcase I am investigating (350 modules, 41 MB wasm after
merge), this makes us go from 5 minutes down to just 19 seconds. With
--no-validation, we drop to 17. (That final number is about how long it
takes to run wasm-opt 350 times to round-trip each of those modules,
so it is shows wasm-merge's merging is very fast.)

Author: kripken

src/tools/wasm-merge.cpp

@@ -91,6 +91,11 @@
 // merged, and at the end we traverse the entire merged module once to fuse
 // imports and exports.
 //
+// Debugging: Set BINARYEN_PASS_DEBUG=1 in the env to get validation after each
+// merging of a module (like pass-debug mode for the pass runner, this does
+// expensive work after each incremental operation). This can take quadratic
+// time, so we do not do it by default.
+//
 
 #include "ir/module-utils.h"
 #include "ir/names.h"
@@ -418,7 +423,7 @@ void checkLimit(bool& valid, const char* kind, T* export_, T* import) {
 
 // Find pairs of matching imports and exports, and make uses of the import refer
 // to the exported item (which has been merged into the module).
-void fuseImportsAndExports() {
+void fuseImportsAndExports(const PassOptions& options) {
   // First, scan the exports and build a map. We build a map of [module name] to
   // [export name => internal name]. For example, consider this module:
   //
@@ -466,80 +471,86 @@ void fuseImportsAndExports() {
     }
   });
 
-  // Make sure that the export types match the import types.
-  bool valid = true;
-  ModuleUtils::iterImportedFunctions(merged, [&](Function* import) {
-    auto internalName = kindModuleExportMaps[ExternalKind::Function]
-                                            [import->module][import->base];
-    if (internalName.is()) {
-      auto* export_ = merged.getFunction(internalName);
-      if (!HeapType::isSubType(export_->type, import->type)) {
-        reportTypeMismatch(valid, "function", import);
-        std::cerr << "type " << export_->type << " is not a subtype of "
-                  << import->type << ".\n";
-      }
-    }
-  });
-  ModuleUtils::iterImportedTables(merged, [&](Table* import) {
-    auto internalName =
-      kindModuleExportMaps[ExternalKind::Table][import->module][import->base];
-    if (internalName.is()) {
-      auto* export_ = merged.getTable(internalName);
-      checkLimit(valid, "table", export_, import);
-      if (export_->type != import->type) {
-        reportTypeMismatch(valid, "table", import);
-        std::cerr << "export type " << export_->type
-                  << " is different from import type " << import->type << ".\n";
-      }
-    }
-  });
-  ModuleUtils::iterImportedMemories(merged, [&](Memory* import) {
-    auto internalName =
-      kindModuleExportMaps[ExternalKind::Memory][import->module][import->base];
-    if (internalName.is()) {
-      auto* export_ = merged.getMemory(internalName);
-      if (export_->is64() != import->is64()) {
-        reportTypeMismatch(valid, "memory", import);
-        std::cerr << "index type should match.\n";
+  if (options.validate) {
+    // Make sure that the export types match the import types.
+    bool valid = true;
+    ModuleUtils::iterImportedFunctions(merged, [&](Function* import) {
+      auto internalName = kindModuleExportMaps[ExternalKind::Function]
+                                              [import->module][import->base];
+      if (internalName.is()) {
+        auto* export_ = merged.getFunction(internalName);
+        if (!HeapType::isSubType(export_->type, import->type)) {
+          reportTypeMismatch(valid, "function", import);
+          std::cerr << "type " << export_->type << " is not a subtype of "
+                    << import->type << ".\n";
+        }
       }
-      checkLimit(valid, "memory", export_, import);
-    }
-  });
-  ModuleUtils::iterImportedGlobals(merged, [&](Global* import) {
-    auto internalName =
-      kindModuleExportMaps[ExternalKind::Global][import->module][import->base];
-    if (internalName.is()) {
-      auto* export_ = merged.getGlobal(internalName);
-      if (export_->mutable_ != import->mutable_) {
-        reportTypeMismatch(valid, "global", import);
-        std::cerr << "mutability should match.\n";
+    });
+    ModuleUtils::iterImportedTables(merged, [&](Table* import) {
+      auto internalName =
+        kindModuleExportMaps[ExternalKind::Table][import->module][import->base];
+      if (internalName.is()) {
+        auto* export_ = merged.getTable(internalName);
+        checkLimit(valid, "table", export_, import);
+        if (export_->type != import->type) {
+          reportTypeMismatch(valid, "table", import);
+          std::cerr << "export type " << export_->type
+                    << " is different from import type " << import->type
+                    << ".\n";
+        }
       }
-      if (export_->mutable_ && export_->type != import->type) {
-        reportTypeMismatch(valid, "global", import);
-        std::cerr << "export type " << export_->type
-                  << " is different from import type " << import->type << ".\n";
+    });
+    ModuleUtils::iterImportedMemories(merged, [&](Memory* import) {
+      auto internalName = kindModuleExportMaps[ExternalKind::Memory]
+                                              [import->module][import->base];
+      if (internalName.is()) {
+        auto* export_ = merged.getMemory(internalName);
+        if (export_->is64() != import->is64()) {
+          reportTypeMismatch(valid, "memory", import);
+          std::cerr << "index type should match.\n";
+        }
+        checkLimit(valid, "memory", export_, import);
       }
-      if (!export_->mutable_ && !Type::isSubType(export_->type, import->type)) {
-        reportTypeMismatch(valid, "global", import);
-        std::cerr << "type " << export_->type << " is not a subtype of "
-                  << import->type << ".\n";
+    });
+    ModuleUtils::iterImportedGlobals(merged, [&](Global* import) {
+      auto internalName = kindModuleExportMaps[ExternalKind::Global]
+                                              [import->module][import->base];
+      if (internalName.is()) {
+        auto* export_ = merged.getGlobal(internalName);
+        if (export_->mutable_ != import->mutable_) {
+          reportTypeMismatch(valid, "global", import);
+          std::cerr << "mutability should match.\n";
+        }
+        if (export_->mutable_ && export_->type != import->type) {
+          reportTypeMismatch(valid, "global", import);
+          std::cerr << "export type " << export_->type
+                    << " is different from import type " << import->type
+                    << ".\n";
+        }
+        if (!export_->mutable_ &&
+            !Type::isSubType(export_->type, import->type)) {
+          reportTypeMismatch(valid, "global", import);
+          std::cerr << "type " << export_->type << " is not a subtype of "
+                    << import->type << ".\n";
+        }
       }
-    }
-  });
-  ModuleUtils::iterImportedTags(merged, [&](Tag* import) {
-    auto internalName =
-      kindModuleExportMaps[ExternalKind::Tag][import->module][import->base];
-    if (internalName.is()) {
-      auto* export_ = merged.getTag(internalName);
-      if (export_->type != import->type) {
-        reportTypeMismatch(valid, "tag", import);
-        std::cerr << "export type " << export_->type
-                  << " is different from import type " << import->type << ".\n";
+    });
+    ModuleUtils::iterImportedTags(merged, [&](Tag* import) {
+      auto internalName =
+        kindModuleExportMaps[ExternalKind::Tag][import->module][import->base];
+      if (internalName.is()) {
+        auto* export_ = merged.getTag(internalName);
+        if (export_->type != import->type) {
+          reportTypeMismatch(valid, "tag", import);
+          std::cerr << "export type " << export_->type
+                    << " is different from import type " << import->type
+                    << ".\n";
+        }
       }
+    });
+    if (!valid) {
+      Fatal() << "import/export mismatches";
     }
-  });
-  if (!valid) {
-    Fatal() << "import/export mismatches";
   }
 
   // Update the things we found.
@@ -730,18 +741,29 @@ Input source maps can be specified by adding an -ism option right after the modu
       // This is a later module: do a full merge.
       mergeInto(*currModule, inputFileName);
 
-      if (options.passOptions.validate) {
-        if (!WasmValidator().validate(merged)) {
+      // Validate after each merged module, when we are in pass-debug mode
+      // (this can be quadratic time).
+      if (PassRunner::getPassDebug()) {
+        std::cerr << "[WasmMerge]   merged : " << inputFile << '\n';
+        if (options.passOptions.validate && !WasmValidator().validate(merged)) {
           std::cout << merged << '\n';
-          Fatal() << "error in validating merged after: " << inputFile;
+          Fatal() << "error in validating after: " << inputFile;
         }
       }
     }
   }
 
+  // If we didn't validate after each merged module, validate once at the very
+  // end. This won't catch problems at the earliest point, but is still useful.
+  if (!PassRunner::getPassDebug() && options.passOptions.validate &&
+      !WasmValidator().validate(merged)) {
+    std::cout << merged << '\n';
+    Fatal() << "error in validating final merged";
+  }
+
   // Fuse imports and exports now that everything is all together in the merged
   // module.
-  fuseImportsAndExports();
+  fuseImportsAndExports(options.passOptions);
 
   {
     PassRunner passRunner(&merged);