wasm-emscripten-finalize: Make EM_ASM modifications optional (#3044)

wasm-emscripten-finalize renames EM_ASM calls to have the signature in
the name. This isn't actually useful - emscripten doesn't benefit from that. I
think it was optimized in fastcomp, and in upstream we copied the general
form but not the optimizations, and then EM_JS came along which is
easier to optimize anyhow.

This PR makes those changes optional: when not doing them, it just
leaves the calls as they are. Emscripten will need some changes to
handle that, but those are simple.

For convenience this adds a flag to "minimize wasm changes". The idea
is that this flag avoids needing a double-roll or other inconvenience
as the changes need to happen in tandem on the emscripten side.
The same flag can be reused for later changes similar to this one.
When they are all done we can remove the flag. (Note how the code
ifdefed by the flag can be removed once we no longer need the old
way of doing things - that is, the new approach is simpler on the
binaryen side).

See #3043

3 files changed, 42 insertions, 13 deletions

diff --git a/src/tools/wasm-emscripten-finalize.cpp b/src/tools/wasm-emscripten-finalize.cpp
index ce1da4c9e..960c6870c 100644
--- a/src/tools/wasm-emscripten-finalize.cpp
+++ b/src/tools/wasm-emscripten-finalize.cpp
@@ -56,6 +56,7 @@ int main(int argc, const char* argv[]) {
   bool checkStackOverflow = false;
   uint64_t globalBase = INVALID_BASE;
   bool standaloneWasm = false;
+  bool minimizeWasmChanges = false;
 
   ToolOptions options("wasm-emscripten-finalize",
                       "Performs Emscripten-specific transforms on .wasm files");
@@ -163,6 +164,15 @@ int main(int argc, const char* argv[]) {
          [&standaloneWasm](Options* o, const std::string&) {
            standaloneWasm = true;
          })
+    .add("--minimize-wasm-changes",
+         "",
+         "Modify the wasm as little as possible. This is useful during "
+         "development as we reduce the number of changes to the wasm, as it "
+         "lets emscripten control how much modifications to do.",
+         Options::Arguments::Zero,
+         [&minimizeWasmChanges](Options* o, const std::string&) {
+           minimizeWasmChanges = true;
+         })
     .add_positional("INFILE",
                     Options::Arguments::One,
                     [&infile](Options* o, const std::string& argument) {
@@ -222,6 +232,7 @@ int main(int argc, const char* argv[]) {
   EmscriptenGlueGenerator generator(wasm);
   generator.setStandalone(standaloneWasm);
   generator.setSideModule(sideModule);
+  generator.setMinimizeWasmChanges(minimizeWasmChanges);
 
   generator.fixInvokeFunctionNames();
 
diff --git a/src/wasm-emscripten.h b/src/wasm-emscripten.h
index 395c76a16..fe8b8ed36 100644
--- a/src/wasm-emscripten.h
+++ b/src/wasm-emscripten.h
@@ -35,6 +35,9 @@ public:
 
   void setStandalone(bool standalone_) { standalone = standalone_; }
   void setSideModule(bool sideModule_) { sideModule = sideModule_; }
+  void setMinimizeWasmChanges(bool minimizeWasmChanges_) {
+    minimizeWasmChanges = minimizeWasmChanges_;
+  }
 
   Function* generateMemoryGrowthFunction();
   Function* generateAssignGOTEntriesFunction();
@@ -71,6 +74,7 @@ private:
   bool useStackPointerGlobal;
   bool standalone;
   bool sideModule;
+  bool minimizeWasmChanges;
   // Used by generateDynCallThunk to track all the dynCall functions created
   // so far.
   std::unordered_set<Signature> sigs;
diff --git a/src/wasm/wasm-emscripten.cpp b/src/wasm/wasm-emscripten.cpp
index e4664e645..f4da0e6e7 100644
--- a/src/wasm/wasm-emscripten.cpp
+++ b/src/wasm/wasm-emscripten.cpp
@@ -320,6 +320,7 @@ std::string proxyingSuffix(Proxying proxy) {
 
 struct AsmConstWalker : public LinearExecutionWalker<AsmConstWalker> {
   Module& wasm;
+  bool minimizeWasmChanges;
   std::vector<Address> segmentOffsets; // segment index => address offset
 
   struct AsmConst {
@@ -334,8 +335,9 @@ struct AsmConstWalker : public LinearExecutionWalker<AsmConstWalker> {
   // last sets in the current basic block, per index
   std::map<Index, LocalSet*> sets;
 
-  AsmConstWalker(Module& _wasm)
-    : wasm(_wasm), segmentOffsets(getSegmentOffsets(wasm)) {}
+  AsmConstWalker(Module& _wasm, bool minimizeWasmChanges)
+    : wasm(_wasm), minimizeWasmChanges(minimizeWasmChanges),
+      segmentOffsets(getSegmentOffsets(wasm)) {}
 
   void noteNonLinear(Expression* curr);
 
@@ -426,7 +428,9 @@ void AsmConstWalker::visitCall(Call* curr) {
   int32_t address = value->value.geti32();
   auto code = codeForConstAddr(wasm, segmentOffsets, address);
   auto& asmConst = createAsmConst(address, code, sig, importName);
-  fixupName(curr->target, baseSig, asmConst.proxy);
+  if (!minimizeWasmChanges) {
+    fixupName(curr->target, baseSig, asmConst.proxy);
+  }
 }
 
 Proxying AsmConstWalker::proxyType(Name name) {
@@ -439,6 +443,9 @@ Proxying AsmConstWalker::proxyType(Name name) {
 }
 
 void AsmConstWalker::visitTable(Table* curr) {
+  if (minimizeWasmChanges) {
+    return;
+  }
   for (auto& segment : curr->segments) {
     for (auto& name : segment.data) {
       auto* func = wasm.getFunction(name);
@@ -515,24 +522,30 @@ void AsmConstWalker::addImports() {
   }
 }
 
-AsmConstWalker fixEmAsmConstsAndReturnWalker(Module& wasm) {
+static AsmConstWalker fixEmAsmConstsAndReturnWalker(Module& wasm,
+                                                    bool minimizeWasmChanges) {
   // Collect imports to remove
   // This would find our generated functions if we ran it later
   std::vector<Name> toRemove;
-  for (auto& import : wasm.functions) {
-    if (import->imported() && import->base.hasSubstring(EM_ASM_PREFIX)) {
-      toRemove.push_back(import->name);
+  if (!minimizeWasmChanges) {
+    for (auto& import : wasm.functions) {
+      if (import->imported() && import->base.hasSubstring(EM_ASM_PREFIX)) {
+        toRemove.push_back(import->name);
+      }
     }
   }
 
   // Walk the module, generate _sig versions of EM_ASM functions
-  AsmConstWalker walker(wasm);
+  AsmConstWalker walker(wasm, minimizeWasmChanges);
   walker.process();
 
-  // Remove the base functions that we didn't generate
-  for (auto importName : toRemove) {
-    wasm.removeFunction(importName);
+  if (!minimizeWasmChanges) {
+    // Remove the base functions that we didn't generate
+    for (auto importName : toRemove) {
+      wasm.removeFunction(importName);
+    }
   }
+
   return walker;
 }
 
@@ -754,7 +767,8 @@ std::string EmscriptenGlueGenerator::generateEmscriptenMetadata(
   std::stringstream meta;
   meta << "{\n";
 
-  AsmConstWalker emAsmWalker = fixEmAsmConstsAndReturnWalker(wasm);
+  AsmConstWalker emAsmWalker =
+    fixEmAsmConstsAndReturnWalker(wasm, minimizeWasmChanges);
 
   // print
   commaFirst = true;
@@ -810,7 +824,7 @@ std::string EmscriptenGlueGenerator::generateEmscriptenMetadata(
   commaFirst = true;
   ModuleUtils::iterImportedFunctions(wasm, [&](Function* import) {
     if (emJsWalker.codeByName.count(import->base.str) == 0 &&
-        !import->base.startsWith(EM_ASM_PREFIX.str) &&
+        (minimizeWasmChanges || !import->base.startsWith(EM_ASM_PREFIX.str)) &&
         !import->base.startsWith("invoke_")) {
       if (declares.insert(import->base.str).second) {
         meta << nextElement() << '"' << import->base.str << '"';