wasm2js: Avoid 64-bit scratch memory helpers in wasm-intrinsics (#2926)

That code originally used memory location 1024 to save 64 bits of
data (as that is what rust does apparently). We refactored it
manually to instead use a scratch memory helper, which is safer.
However, that 64-bit function ends up legalized, which actually
changes the interface between the module and the outside,
which is confusing and causes problems with optimizations
that can remove the getTempRet0 imports, see
emscripten-core/emscripten#11456

Instead, just use a global i64 to stash those bits. This requires
adding support for copying globals from the intrinsics module,
but otherwise seems simpler overall.

5 files changed, 17 insertions, 33 deletions

diff --git a/src/abi/js.h b/src/abi/js.h
index c8ac94407..118a2c981 100644
--- a/src/abi/js.h
+++ b/src/abi/js.h
@@ -38,8 +38,6 @@ namespace wasm2js {
 
 extern cashew::IString SCRATCH_LOAD_I32;
 extern cashew::IString SCRATCH_STORE_I32;
-extern cashew::IString SCRATCH_LOAD_I64;
-extern cashew::IString SCRATCH_STORE_I64;
 extern cashew::IString SCRATCH_LOAD_F32;
 extern cashew::IString SCRATCH_STORE_F32;
 extern cashew::IString SCRATCH_LOAD_F64;
@@ -76,8 +74,6 @@ inline void ensureHelpers(Module* wasm,
 
   ensureImport(SCRATCH_LOAD_I32, {Type::i32}, Type::i32);
   ensureImport(SCRATCH_STORE_I32, {Type::i32, Type::i32}, Type::none);
-  ensureImport(SCRATCH_LOAD_I64, {}, Type::i64);
-  ensureImport(SCRATCH_STORE_I64, {Type::i64}, Type::none);
   ensureImport(SCRATCH_LOAD_F32, {}, Type::f32);
   ensureImport(SCRATCH_STORE_F32, {Type::f32}, Type::none);
   ensureImport(SCRATCH_LOAD_F64, {}, Type::f64);
@@ -98,7 +94,6 @@ inline void ensureHelpers(Module* wasm,
 
 inline bool isHelper(cashew::IString name) {
   return name == SCRATCH_LOAD_I32 || name == SCRATCH_STORE_I32 ||
-         name == SCRATCH_LOAD_I64 || name == SCRATCH_STORE_I64 ||
          name == SCRATCH_LOAD_F32 || name == SCRATCH_STORE_F32 ||
          name == SCRATCH_LOAD_F64 || name == SCRATCH_STORE_F64 ||
          name == ATOMIC_WAIT_I32 || name == MEMORY_INIT ||
diff --git a/src/asmjs/shared-constants.cpp b/src/asmjs/shared-constants.cpp
index 8a13cf55c..37594108a 100644
--- a/src/asmjs/shared-constants.cpp
+++ b/src/asmjs/shared-constants.cpp
@@ -113,8 +113,6 @@ namespace wasm2js {
 
 cashew::IString SCRATCH_LOAD_I32("wasm2js_scratch_load_i32");
 cashew::IString SCRATCH_STORE_I32("wasm2js_scratch_store_i32");
-cashew::IString SCRATCH_LOAD_I64("wasm2js_scratch_load_i64");
-cashew::IString SCRATCH_STORE_I64("wasm2js_scratch_store_i64");
 cashew::IString SCRATCH_LOAD_F32("wasm2js_scratch_load_f32");
 cashew::IString SCRATCH_STORE_F32("wasm2js_scratch_store_f32");
 cashew::IString SCRATCH_LOAD_F64("wasm2js_scratch_load_f64");
diff --git a/src/passes/RemoveNonJSOps.cpp b/src/passes/RemoveNonJSOps.cpp
index 3c1947725..e25a834b0 100644
--- a/src/passes/RemoveNonJSOps.cpp
+++ b/src/passes/RemoveNonJSOps.cpp
@@ -107,6 +107,11 @@ struct RemoveNonJSOpsPass : public WalkerPass<PostWalker<RemoveNonJSOpsPass>> {
       neededFunctions.clear();
     }
 
+    // Copy all the globals in the intrinsics module
+    for (auto& global : intrinsicsModule.globals) {
+      ModuleUtils::copyGlobal(global.get(), *module);
+    }
+
     // Intrinsics may use memory, so ensure the module has one.
     MemoryUtils::ensureExists(module->memory);
 
diff --git a/src/passes/wasm-intrinsics.wat b/src/passes/wasm-intrinsics.wat
index f54547a0d..76e900734 100644
--- a/src/passes/wasm-intrinsics.wat
+++ b/src/passes/wasm-intrinsics.wat
@@ -6,9 +6,8 @@
 ;; (aka inlining and whatnot)
 ;;
 ;; LOCAL MODS done by hand afterwards:
-;;  * Remove hardcoded address 1024 (apparently a free memory location rustc
-;;    thinks is ok to use?); add intrinsic functions, which load/store to
-;;    special scratch space, wasm2js_scratch_load_i32 etc.
+;;  * Remove hardcoded address 1024 which was used for temporary data; instead
+;;    add $wasm-intrinsics-temp-i64 global for that.
 ;;  * Fix function type of __wasm_ctz_i64, which was wrong somehow,
 ;;    i32, i32 => i32 instead of i64 => i64
 ;;
@@ -22,8 +21,6 @@
  (type $4 (func (param i32 i32) (result i32)))
  (type $5 (func (param i64) (result i64)))
  (import "env" "memory" (memory $0 17))
- (import "env" "wasm2js_scratch_load_i64" (func $wasm2js_scratch_load_i64 (result i64)))
- (import "env" "wasm2js_scratch_store_i64" (func $wasm2js_scratch_store_i64 (param i64)))
  (export "__wasm_i64_sdiv" (func $__wasm_i64_sdiv))
  (export "__wasm_i64_udiv" (func $__wasm_i64_udiv))
  (export "__wasm_i64_srem" (func $__wasm_i64_srem))
@@ -41,6 +38,7 @@
  (export "__wasm_nearest_f64" (func $__wasm_nearest_f64))
  (export "__wasm_popcnt_i32" (func $__wasm_popcnt_i32))
  (export "__wasm_popcnt_i64" (func $__wasm_popcnt_i64))
+ (global $__wasm-intrinsics-temp-i64 (mut i64) (i64.const 0))
 
  ;; lowering of the i32.popcnt instruction, counts the number of bits set in the
  ;; input and returns the result
@@ -137,7 +135,7 @@
     (local.get $var$1)
    )
   )
-  (call $wasm2js_scratch_load_i64)
+  (global.get $__wasm-intrinsics-temp-i64)
  )
  ;; lowering of the i64.mul instruction, return $var0 * $var$1
  (func $__wasm_i64_mul (; 4 ;) (type $0) (param $var$0 i64) (param $var$1 i64) (result i64)
@@ -578,7 +576,7 @@
                (i64.const 4294967296)
               )
              )
-             (call $wasm2js_scratch_store_i64
+             (global.set $__wasm-intrinsics-temp-i64
               (i64.extend_i32_u
                (i32.sub
                 (local.tee $var$2
@@ -639,7 +637,7 @@
               (local.get $var$3)
              )
             )
-            (call $wasm2js_scratch_store_i64
+            (global.set $__wasm-intrinsics-temp-i64
              (i64.or
               (i64.shl
                (i64.extend_i32_u
@@ -719,7 +717,7 @@
          )
          (br $label$3)
         )
-        (call $wasm2js_scratch_store_i64
+        (global.set $__wasm-intrinsics-temp-i64
          (i64.shl
           (i64.extend_i32_u
            (i32.sub
@@ -761,7 +759,7 @@
        )
        (br $label$2)
       )
-      (call $wasm2js_scratch_store_i64
+      (global.set $__wasm-intrinsics-temp-i64
        (i64.extend_i32_u
         (i32.and
          (local.get $var$4)
@@ -892,7 +890,7 @@
       )
      )
     )
-    (call $wasm2js_scratch_store_i64
+    (global.set $__wasm-intrinsics-temp-i64
      (local.get $var$5)
     )
     (return
@@ -905,7 +903,7 @@
      )
     )
    )
-   (call $wasm2js_scratch_store_i64
+   (global.set $__wasm-intrinsics-temp-i64
     (local.get $var$0)
    )
    (local.set $var$0
diff --git a/src/wasm2js.h b/src/wasm2js.h
index a6f27eff1..6b4ad697f 100644
--- a/src/wasm2js.h
+++ b/src/wasm2js.h
@@ -283,6 +283,8 @@ Ref Wasm2JSBuilder::processWasm(Module* wasm, Name funcName) {
   {
     PassRunner runner(wasm, options);
     runner.add(make_unique<AutoDrop>());
+    // TODO: only legalize if necessary - emscripten would already do so, and
+    //       likely other toolchains. but spec test suite needs that.
     runner.add("legalize-js-interface");
     // First up remove as many non-JS operations we can, including things like
     // 64-bit integer multiplication/division, `f32.nearest` instructions, etc.
@@ -2378,20 +2380,6 @@ void Wasm2JSGlue::emitSpecialSupport() {
     return i32ScratchView[index];
   }
       )";
-    } else if (import->base == ABI::wasm2js::SCRATCH_STORE_I64) {
-      out << R"(
-  function legalimport$wasm2js_scratch_store_i64(low, high) {
-    i32ScratchView[0] = low;
-    i32ScratchView[1] = high;
-  }
-      )";
-    } else if (import->base == ABI::wasm2js::SCRATCH_LOAD_I64) {
-      out << R"(
-  function legalimport$wasm2js_scratch_load_i64() {
-    if (typeof setTempRet0 === 'function') setTempRet0(i32ScratchView[1]);
-    return i32ScratchView[0];
-  }
-      )";
     } else if (import->base == ABI::wasm2js::SCRATCH_STORE_F32) {
       out << R"(
   function wasm2js_scratch_store_f32(value) {