Skip to content

Commit 8540db8

Browse files
kripkenkripken
authored
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.
1 parent baec7c0 commit 8540db8

10 files changed

+317
-1024
lines changed

‎src/abi/js.h

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,6 @@ namespace wasm2js {
3838

3939
extern cashew::IString SCRATCH_LOAD_I32;
4040
extern cashew::IString SCRATCH_STORE_I32;
41-
extern cashew::IString SCRATCH_LOAD_I64;
42-
extern cashew::IString SCRATCH_STORE_I64;
4341
extern cashew::IString SCRATCH_LOAD_F32;
4442
extern cashew::IString SCRATCH_STORE_F32;
4543
extern cashew::IString SCRATCH_LOAD_F64;
@@ -76,8 +74,6 @@ inline void ensureHelpers(Module* wasm,
7674

7775
ensureImport(SCRATCH_LOAD_I32, {Type::i32}, Type::i32);
7876
ensureImport(SCRATCH_STORE_I32, {Type::i32, Type::i32}, Type::none);
79-
ensureImport(SCRATCH_LOAD_I64, {}, Type::i64);
80-
ensureImport(SCRATCH_STORE_I64, {Type::i64}, Type::none);
8177
ensureImport(SCRATCH_LOAD_F32, {}, Type::f32);
8278
ensureImport(SCRATCH_STORE_F32, {Type::f32}, Type::none);
8379
ensureImport(SCRATCH_LOAD_F64, {}, Type::f64);
@@ -98,7 +94,6 @@ inline void ensureHelpers(Module* wasm,
9894

9995
inline bool isHelper(cashew::IString name) {
10096
return name == SCRATCH_LOAD_I32 || name == SCRATCH_STORE_I32 ||
101-
name == SCRATCH_LOAD_I64 || name == SCRATCH_STORE_I64 ||
10297
name == SCRATCH_LOAD_F32 || name == SCRATCH_STORE_F32 ||
10398
name == SCRATCH_LOAD_F64 || name == SCRATCH_STORE_F64 ||
10499
name == ATOMIC_WAIT_I32 || name == MEMORY_INIT ||

‎src/asmjs/shared-constants.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,6 @@ namespace wasm2js {
113113

114114
cashew::IString SCRATCH_LOAD_I32("wasm2js_scratch_load_i32");
115115
cashew::IString SCRATCH_STORE_I32("wasm2js_scratch_store_i32");
116-
cashew::IString SCRATCH_LOAD_I64("wasm2js_scratch_load_i64");
117-
cashew::IString SCRATCH_STORE_I64("wasm2js_scratch_store_i64");
118116
cashew::IString SCRATCH_LOAD_F32("wasm2js_scratch_load_f32");
119117
cashew::IString SCRATCH_STORE_F32("wasm2js_scratch_store_f32");
120118
cashew::IString SCRATCH_LOAD_F64("wasm2js_scratch_load_f64");

‎src/passes/RemoveNonJSOps.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,11 @@ struct RemoveNonJSOpsPass : public WalkerPass<PostWalker<RemoveNonJSOpsPass>> {
107107
neededFunctions.clear();
108108
}
109109

110+
// Copy all the globals in the intrinsics module
111+
for (auto& global : intrinsicsModule.globals) {
112+
ModuleUtils::copyGlobal(global.get(), *module);
113+
}
114+
110115
// Intrinsics may use memory, so ensure the module has one.
111116
MemoryUtils::ensureExists(module->memory);
112117

‎src/passes/wasm-intrinsics.wat

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,8 @@
66
;; (aka inlining and whatnot)
77
;;
88
;; LOCAL MODS done by hand afterwards:
9-
;; * Remove hardcoded address 1024 (apparently a free memory location rustc
10-
;; thinks is ok to use?); add intrinsic functions, which load/store to
11-
;; special scratch space, wasm2js_scratch_load_i32 etc.
9+
;; * Remove hardcoded address 1024 which was used for temporary data; instead
10+
;; add $wasm-intrinsics-temp-i64 global for that.
1211
;; * Fix function type of __wasm_ctz_i64, which was wrong somehow,
1312
;; i32, i32 => i32 instead of i64 => i64
1413
;;
@@ -22,8 +21,6 @@
2221
(type $4 (func (param i32 i32) (result i32)))
2322
(type $5 (func (param i64) (result i64)))
2423
(import "env" "memory" (memory $0 17))
25-
(import "env" "wasm2js_scratch_load_i64" (func $wasm2js_scratch_load_i64 (result i64)))
26-
(import "env" "wasm2js_scratch_store_i64" (func $wasm2js_scratch_store_i64 (param i64)))
2724
(export "__wasm_i64_sdiv" (func $__wasm_i64_sdiv))
2825
(export "__wasm_i64_udiv" (func $__wasm_i64_udiv))
2926
(export "__wasm_i64_srem" (func $__wasm_i64_srem))
@@ -41,6 +38,7 @@
4138
(export "__wasm_nearest_f64" (func $__wasm_nearest_f64))
4239
(export "__wasm_popcnt_i32" (func $__wasm_popcnt_i32))
4340
(export "__wasm_popcnt_i64" (func $__wasm_popcnt_i64))
41+
(global $__wasm-intrinsics-temp-i64 (mut i64) (i64.const 0))
4442

4543
;; lowering of the i32.popcnt instruction, counts the number of bits set in the
4644
;; input and returns the result
@@ -137,7 +135,7 @@
137135
(local.get $var$1)
138136
)
139137
)
140-
(call $wasm2js_scratch_load_i64)
138+
(global.get $__wasm-intrinsics-temp-i64)
141139
)
142140
;; lowering of the i64.mul instruction, return $var0 * $var$1
143141
(func $__wasm_i64_mul (; 4 ;) (type $0) (param $var$0 i64) (param $var$1 i64) (result i64)
@@ -578,7 +576,7 @@
578576
(i64.const 4294967296)
579577
)
580578
)
581-
(call $wasm2js_scratch_store_i64
579+
(global.set $__wasm-intrinsics-temp-i64
582580
(i64.extend_i32_u
583581
(i32.sub
584582
(local.tee $var$2
@@ -639,7 +637,7 @@
639637
(local.get $var$3)
640638
)
641639
)
642-
(call $wasm2js_scratch_store_i64
640+
(global.set $__wasm-intrinsics-temp-i64
643641
(i64.or
644642
(i64.shl
645643
(i64.extend_i32_u
@@ -719,7 +717,7 @@
719717
)
720718
(br $label$3)
721719
)
722-
(call $wasm2js_scratch_store_i64
720+
(global.set $__wasm-intrinsics-temp-i64
723721
(i64.shl
724722
(i64.extend_i32_u
725723
(i32.sub
@@ -761,7 +759,7 @@
761759
)
762760
(br $label$2)
763761
)
764-
(call $wasm2js_scratch_store_i64
762+
(global.set $__wasm-intrinsics-temp-i64
765763
(i64.extend_i32_u
766764
(i32.and
767765
(local.get $var$4)
@@ -892,7 +890,7 @@
892890
)
893891
)
894892
)
895-
(call $wasm2js_scratch_store_i64
893+
(global.set $__wasm-intrinsics-temp-i64
896894
(local.get $var$5)
897895
)
898896
(return
@@ -905,7 +903,7 @@
905903
)
906904
)
907905
)
908-
(call $wasm2js_scratch_store_i64
906+
(global.set $__wasm-intrinsics-temp-i64
909907
(local.get $var$0)
910908
)
911909
(local.set $var$0

‎src/wasm2js.h

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,8 @@ Ref Wasm2JSBuilder::processWasm(Module* wasm, Name funcName) {
283283
{
284284
PassRunner runner(wasm, options);
285285
runner.add(make_unique<AutoDrop>());
286+
// TODO: only legalize if necessary - emscripten would already do so, and
287+
// likely other toolchains. but spec test suite needs that.
286288
runner.add("legalize-js-interface");
287289
// First up remove as many non-JS operations we can, including things like
288290
// 64-bit integer multiplication/division, `f32.nearest` instructions, etc.
@@ -2376,20 +2378,6 @@ void Wasm2JSGlue::emitSpecialSupport() {
23762378
out << R"(
23772379
function wasm2js_scratch_load_i32(index) {
23782380
return i32ScratchView[index];
2379-
}
2380-
)";
2381-
} else if (import->base == ABI::wasm2js::SCRATCH_STORE_I64) {
2382-
out << R"(
2383-
function legalimport$wasm2js_scratch_store_i64(low, high) {
2384-
i32ScratchView[0] = low;
2385-
i32ScratchView[1] = high;
2386-
}
2387-
)";
2388-
} else if (import->base == ABI::wasm2js::SCRATCH_LOAD_I64) {
2389-
out << R"(
2390-
function legalimport$wasm2js_scratch_load_i64() {
2391-
if (typeof setTempRet0 === 'function') setTempRet0(i32ScratchView[1]);
2392-
return i32ScratchView[0];
23932381
}
23942382
)";
23952383
} else if (import->base == ABI::wasm2js::SCRATCH_STORE_F32) {

‎test/passes/remove-non-js-ops.txt

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,21 +8,17 @@
88
(type $i32_=>_none (func (param i32)))
99
(type $i32_i32_=>_none (func (param i32 i32)))
1010
(type $i32_i32_i32_i32_=>_none (func (param i32 i32 i32 i32)))
11-
(type $i64_=>_none (func (param i64)))
1211
(type $f32_=>_none (func (param f32)))
1312
(type $f64_=>_none (func (param f64)))
1413
(type $none_=>_i32 (func (result i32)))
1514
(type $i32_i32_i32_i32_=>_i32 (func (param i32 i32 i32 i32) (result i32)))
1615
(type $i32_i32_i32_i32_i32_i32_=>_i32 (func (param i32 i32 i32 i32 i32 i32) (result i32)))
17-
(type $none_=>_i64 (func (result i64)))
1816
(type $none_=>_f32 (func (result f32)))
1917
(type $f32_f32_=>_f32 (func (param f32 f32) (result f32)))
2018
(type $none_=>_f64 (func (result f64)))
2119
(type $f64_f64_=>_f64 (func (param f64 f64) (result f64)))
2220
(import "env" "wasm2js_scratch_load_i32" (func $wasm2js_scratch_load_i32 (param i32) (result i32)))
2321
(import "env" "wasm2js_scratch_store_i32" (func $wasm2js_scratch_store_i32 (param i32 i32)))
24-
(import "env" "wasm2js_scratch_load_i64" (func $wasm2js_scratch_load_i64 (result i64)))
25-
(import "env" "wasm2js_scratch_store_i64" (func $wasm2js_scratch_store_i64 (param i64)))
2622
(import "env" "wasm2js_scratch_load_f32" (func $wasm2js_scratch_load_f32 (result f32)))
2723
(import "env" "wasm2js_scratch_store_f32" (func $wasm2js_scratch_store_f32 (param f32)))
2824
(import "env" "wasm2js_scratch_load_f64" (func $wasm2js_scratch_load_f64 (result f64)))
@@ -35,6 +31,7 @@
3531
(import "env" "wasm2js_atomic_rmw_i64" (func $wasm2js_atomic_rmw_i64 (param i32 i32 i32 i32 i32 i32) (result i32)))
3632
(import "env" "wasm2js_get_stashed_bits" (func $wasm2js_get_stashed_bits (result i32)))
3733
(memory $0 1)
34+
(global $__wasm-intrinsics-temp-i64 (mut i64) (i64.const 0))
3835
(func $copysign64 (param $0 f64) (param $1 f64) (result f64)
3936
(f64.reinterpret_i64
4037
(i64.or
@@ -426,7 +423,7 @@
426423
(i64.const 4294967296)
427424
)
428425
)
429-
(call $wasm2js_scratch_store_i64
426+
(global.set $__wasm-intrinsics-temp-i64
430427
(i64.extend_i32_u
431428
(i32.sub
432429
(local.tee $var$2
@@ -487,7 +484,7 @@
487484
(local.get $var$3)
488485
)
489486
)
490-
(call $wasm2js_scratch_store_i64
487+
(global.set $__wasm-intrinsics-temp-i64
491488
(i64.or
492489
(i64.shl
493490
(i64.extend_i32_u
@@ -567,7 +564,7 @@
567564
)
568565
(br $label$3)
569566
)
570-
(call $wasm2js_scratch_store_i64
567+
(global.set $__wasm-intrinsics-temp-i64
571568
(i64.shl
572569
(i64.extend_i32_u
573570
(i32.sub
@@ -609,7 +606,7 @@
609606
)
610607
(br $label$2)
611608
)
612-
(call $wasm2js_scratch_store_i64
609+
(global.set $__wasm-intrinsics-temp-i64
613610
(i64.extend_i32_u
614611
(i32.and
615612
(local.get $var$4)
@@ -740,7 +737,7 @@
740737
)
741738
)
742739
)
743-
(call $wasm2js_scratch_store_i64
740+
(global.set $__wasm-intrinsics-temp-i64
744741
(local.get $var$5)
745742
)
746743
(return
@@ -753,7 +750,7 @@
753750
)
754751
)
755752
)
756-
(call $wasm2js_scratch_store_i64
753+
(global.set $__wasm-intrinsics-temp-i64
757754
(local.get $var$0)
758755
)
759756
(local.set $var$0
@@ -813,7 +810,7 @@
813810
(local.get $var$1)
814811
)
815812
)
816-
(call $wasm2js_scratch_load_i64)
813+
(global.get $__wasm-intrinsics-temp-i64)
817814
)
818815
(func $__wasm_nearest_f32 (param $var$0 f32) (result f32)
819816
(local $var$1 f32)

0 commit comments

Comments
 (0)