From 40af53b7893bafaa1339e290bf3ed67118c1cc56 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 11 Jan 2024 10:13:20 -0800 Subject: [PATCH 1/3] start --- src/passes/ReorderGlobals.cpp | 6 ++++- src/tools/wasm-merge.cpp | 14 +++++++---- test/lit/merge/global-ordering.wat | 30 +++++++++++++++++++++++ test/lit/merge/global-ordering.wat.second | 3 +++ test/lit/merge/renamings.wat | 11 +++++---- 5 files changed, 53 insertions(+), 11 deletions(-) create mode 100644 test/lit/merge/global-ordering.wat create mode 100644 test/lit/merge/global-ordering.wat.second diff --git a/src/passes/ReorderGlobals.cpp b/src/passes/ReorderGlobals.cpp index d1c26b2ccee..4d39f96b89a 100644 --- a/src/passes/ReorderGlobals.cpp +++ b/src/passes/ReorderGlobals.cpp @@ -58,7 +58,11 @@ struct UseCountScanner : public WalkerPass> { struct ReorderGlobals : public Pass { // Whether to always reorder globals, even if there are very few and the - // benefit is minor. That is useful for testing. + // benefit is minor. That is useful for testing, and also internally in passes + // that use us to reorder them so dependencies appear first (that is, if a + // pass ends up with an earlier global reading a later one, the sorting in + // this pass will reorder them properly; we need to take those depdencies into + // account anyhow when sorting by size). bool always; ReorderGlobals(bool always) : always(always) {} diff --git a/src/tools/wasm-merge.cpp b/src/tools/wasm-merge.cpp index 088317eec6c..0884350cb5b 100644 --- a/src/tools/wasm-merge.cpp +++ b/src/tools/wasm-merge.cpp @@ -586,13 +586,17 @@ Note that filenames and modules names are interleaved (which is hopefully less c // module. fuseImportsAndExports(); - // Remove unused things. This is obviously a useful optimization, but it also - // makes using the output easier: if an import was resolved by an export - // during the merge, then that import will have no more uses and it will be - // optimized out (while if we didn't optimize it out then instantiating the - // module would still be forced to provide something for that import). { PassRunner passRunner(&merged); + // We might have made some globals read from others that now appear after + // them (if the one they read was appended from a later module). Sort them + // to fix that. TODO: we could do this only if we actually append globals + passRunner.add("reorder-globals-always"); + // Remove unused things. This is obviously a useful optimization but it also + // makes using the output easier: if an import was resolved by an export + // during the merge, then that import will have no more uses and it will be + // optimized out (while if we didn't optimize it out then instantiating the + // module would still be forced to provide something for that import). passRunner.add("remove-unused-module-elements"); passRunner.run(); } diff --git a/test/lit/merge/global-ordering.wat b/test/lit/merge/global-ordering.wat new file mode 100644 index 00000000000..71d676f7499 --- /dev/null +++ b/test/lit/merge/global-ordering.wat @@ -0,0 +1,30 @@ +;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. + +;; RUN: wasm-merge %s first %s.second second --rename-export-conflicts -all -S -o - | filecheck %s + +;; After the merge this module's global will read a value from a global that is +;; appended after it, from the second module. Those must be reordered so that +;; we validate, as a global can only read from previous ones. + +(module + (import "second" "second.global.export" (global i32)) + + ;; CHECK: (type $0 (func (result i32))) + + ;; CHECK: (global $second.global i32 (i32.const 42)) + + ;; CHECK: (global $first.global (mut i32) (global.get $second.global)) + (global $first.global (mut i32) (global.get 0)) + + ;; CHECK: (export "run" (func $run)) + + ;; CHECK: (export "second.global.export" (global $second.global)) + + ;; CHECK: (func $run (type $0) (result i32) + ;; CHECK-NEXT: (global.get $first.global) + ;; CHECK-NEXT: ) + (func $run (export "run") (result i32) + ;; Use the global to avoid it being removed. + (global.get $first.global) + ) +) diff --git a/test/lit/merge/global-ordering.wat.second b/test/lit/merge/global-ordering.wat.second new file mode 100644 index 00000000000..7676599e48a --- /dev/null +++ b/test/lit/merge/global-ordering.wat.second @@ -0,0 +1,3 @@ +(module + (global $second.global (export "second.global.export") i32 (i32.const 42)) +) diff --git a/test/lit/merge/renamings.wat b/test/lit/merge/renamings.wat index 6d4de5c65f0..98aac63bef9 100644 --- a/test/lit/merge/renamings.wat +++ b/test/lit/merge/renamings.wat @@ -23,20 +23,21 @@ ;; CHECK: (import "elsewhere" "some.tag" (tag $imported (param f64))) + ;; CHECK: (global $bar_2 i32 (i32.const 4)) + + ;; CHECK: (global $other i32 (i32.const 3)) + + ;; CHECK: (global $bar i32 (i32.const 2)) + ;; CHECK: (global $foo i32 (i32.const 1)) (global $foo i32 (i32.const 1)) ;; This global has a conflict in second.wat, and so second.wat's $bar ;; will be renamed. - ;; CHECK: (global $bar i32 (i32.const 2)) (global $bar i32 (i32.const 2)) ;; This memory has a conflict in second.wat, and so second.wat's $foo ;; will be renamed. - ;; CHECK: (global $other i32 (i32.const 3)) - - ;; CHECK: (global $bar_2 i32 (i32.const 4)) - ;; CHECK: (memory $foo 10 20) (memory $foo 10 20) From ae9b66939edf4c6c15ffcc9266ef51a77d24e325 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 11 Jan 2024 10:13:29 -0800 Subject: [PATCH 2/3] simpl --- test/lit/merge/global-ordering.wat | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/lit/merge/global-ordering.wat b/test/lit/merge/global-ordering.wat index 71d676f7499..2cc3b504b7f 100644 --- a/test/lit/merge/global-ordering.wat +++ b/test/lit/merge/global-ordering.wat @@ -1,6 +1,6 @@ ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. -;; RUN: wasm-merge %s first %s.second second --rename-export-conflicts -all -S -o - | filecheck %s +;; RUN: wasm-merge %s first %s.second second -all -S -o - | filecheck %s ;; After the merge this module's global will read a value from a global that is ;; appended after it, from the second module. Those must be reordered so that From 3ff6006b758288982df14287473dc46dcdd792b4 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 11 Jan 2024 10:15:23 -0800 Subject: [PATCH 3/3] typo --- src/passes/ReorderGlobals.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/passes/ReorderGlobals.cpp b/src/passes/ReorderGlobals.cpp index 4d39f96b89a..9b5940a214b 100644 --- a/src/passes/ReorderGlobals.cpp +++ b/src/passes/ReorderGlobals.cpp @@ -61,8 +61,8 @@ struct ReorderGlobals : public Pass { // benefit is minor. That is useful for testing, and also internally in passes // that use us to reorder them so dependencies appear first (that is, if a // pass ends up with an earlier global reading a later one, the sorting in - // this pass will reorder them properly; we need to take those depdencies into - // account anyhow when sorting by size). + // this pass will reorder them properly; we need to take those dependencies + // into account anyhow here). bool always; ReorderGlobals(bool always) : always(always) {}