From b0bec8fd2adbd7b770cc4e06d7f87779ab64a4a0 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 31 Jan 2024 11:09:55 -0800 Subject: [PATCH 01/12] start --- src/passes/MemoryPacking.cpp | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/src/passes/MemoryPacking.cpp b/src/passes/MemoryPacking.cpp index c475164851d..e88230935aa 100644 --- a/src/passes/MemoryPacking.cpp +++ b/src/passes/MemoryPacking.cpp @@ -34,6 +34,7 @@ #include "ir/utils.h" #include "pass.h" #include "support/space.h" +#include "support/stdckdint.h" #include "wasm-binary.h" #include "wasm-builder.h" #include "wasm.h" @@ -46,6 +47,7 @@ namespace { // will be used instead of memory.init for this range. struct Range { bool isZero; + // The range [start, end) - that is, start is included while end is not. size_t start; size_t end; }; @@ -107,7 +109,8 @@ struct MemoryPacking : public Pass { void dropUnusedSegments(Module* module, std::vector>& segments, ReferrersMap& referrers); - bool canSplit(const std::unique_ptr& segment, + bool canSplit(Module* module, + const std::unique_ptr& segment, const Referrers& referrers); void calculateRanges(const std::unique_ptr& segment, const Referrers& referrers, @@ -161,7 +164,7 @@ void MemoryPacking::run(Module* module) { std::vector ranges; - if (canSplit(segment, currReferrers)) { + if (canSplit(module, segment, currReferrers)) { calculateRanges(segment, currReferrers, ranges); } else { // A single range covers the entire segment. Set isZero to false so the @@ -260,7 +263,8 @@ bool MemoryPacking::canOptimize( return true; } -bool MemoryPacking::canSplit(const std::unique_ptr& segment, +bool MemoryPacking::canSplit(Module* module, + const std::unique_ptr& segment, const Referrers& referrers) { // Don't mess with segments related to llvm coverage tools such as // __llvm_covfun. There segments are expected/parsed by external downstream @@ -277,6 +281,24 @@ bool MemoryPacking::canSplit(const std::unique_ptr& segment, return false; } + // Do not split an active segment that might trap. + if (!getPassOptions().trapsNeverHappen && segment->offset) { + auto* c = segment->offset->dynCast(); + if (!c) { + // We can't tell if this will trap or not. + return false; + } + auto* memory = module->getMemory(segment->memory); + auto memorySize = memory->initial * Memory::kPageSize; + Index start = c->value.getUnsigned(); + Index size = segment->data.size(); + Index end; + if (std::ckd_add(&end, start, size) || end > memorySize) { + // This traps. + return false; + } + } + for (auto* referrer : referrers) { if (auto* curr = referrer->dynCast()) { if (segment->isPassive) { From a1cf59b603d5dddf7e9d96e0e7ce9453a7338a9f Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 31 Jan 2024 11:13:18 -0800 Subject: [PATCH 02/12] test --- test/lit/passes/memory-packing_traps.wast | 36 +++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 test/lit/passes/memory-packing_traps.wast diff --git a/test/lit/passes/memory-packing_traps.wast b/test/lit/passes/memory-packing_traps.wast new file mode 100644 index 00000000000..05eed80e686 --- /dev/null +++ b/test/lit/passes/memory-packing_traps.wast @@ -0,0 +1,36 @@ +;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. + +;; RUN: wasm-opt %s --memory-packing -all --zero-filled-memory -S -o - | filecheck %s +;; RUN: wasm-opt %s --memory-packing -all --zero-filled-memory -tnh -S -o - | filecheck %s --check-prefix=_TNH_ + +;; We should not optimize out a segment that will trap, as that is an effect we +;; need to preserve (unless TrapsNeverHappen). +(module + (memory 1 2 shared) + (data (i32.const -1) "\00") +) + +;; This segment will almost trap, but not. +(module + (memory 1 2 shared) + (data (i32.const 65535) "\00") +) + +;; This one is slightly larger, and will trap. +(module + (memory 1 2 shared) + (data (i32.const 65535) "\00\00") +) + +;; This one's offset is just large enough to trap. +(module + (memory 1 2 shared) + (data (i32.const 65536) "\00") +) + +;; This offset is unknown, so assume the worst. +(module + (import "a" "b" (global $g i32)) + (memory 1 2 shared) + (data (global.get $g) "\00") +) From 5eff14b56a33adb3f8f6a46f7c9c4f6af93b73e8 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 31 Jan 2024 11:16:50 -0800 Subject: [PATCH 03/12] work --- test/lit/passes/memory-packing_traps.wast | 64 ++++++++++++++++++++--- 1 file changed, 56 insertions(+), 8 deletions(-) diff --git a/test/lit/passes/memory-packing_traps.wast b/test/lit/passes/memory-packing_traps.wast index 05eed80e686..a9308299b3d 100644 --- a/test/lit/passes/memory-packing_traps.wast +++ b/test/lit/passes/memory-packing_traps.wast @@ -1,36 +1,84 @@ ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. -;; RUN: wasm-opt %s --memory-packing -all --zero-filled-memory -S -o - | filecheck %s -;; RUN: wasm-opt %s --memory-packing -all --zero-filled-memory -tnh -S -o - | filecheck %s --check-prefix=_TNH_ +;; RUN: foreach %s %t wasm-opt --memory-packing -all --zero-filled-memory -S -o - | filecheck %s +;; RUN: foreach %s %t wasm-opt --memory-packing -all --zero-filled-memory -tnh -S -o - | filecheck %s --check-prefix=_TNH_ -;; We should not optimize out a segment that will trap, as that is an effect we -;; need to preserve (unless TrapsNeverHappen). (module + ;; We should not optimize out a segment that will trap, as that is an effect + ;; we need to preserve (unless TrapsNeverHappen). (memory 1 2 shared) (data (i32.const -1) "\00") ) -;; This segment will almost trap, but not. +;; CHECK: (memory $0 1 2 shared) + +;; CHECK: (data $0 (i32.const -1) "\00") + +;; _TNH_: (memory $0 1 2 shared) (module + ;; We should handle the possible overflow in adding the offset and size, and + ;; see this might trap. + (memory 1 2 shared) + (data (i32.const -2) "\00\00\00") +) + +;; CHECK: (memory $0 1 2 shared) + +;; CHECK: (data $0 (i32.const -2) "\00\00\00") + +;; _TNH_: (memory $0 1 2 shared) +(module + ;; This segment will almost trap, but not. (memory 1 2 shared) (data (i32.const 65535) "\00") ) -;; This one is slightly larger, and will trap. +;; CHECK: (memory $0 1 2 shared) + +;; _TNH_: (memory $0 1 2 shared) (module + ;; This one is slightly larger, and will trap. (memory 1 2 shared) (data (i32.const 65535) "\00\00") ) -;; This one's offset is just large enough to trap. +;; CHECK: (memory $0 1 2 shared) + +;; CHECK: (data $0 (i32.const 65535) "\00\00") + +;; _TNH_: (memory $0 1 2 shared) (module + ;; This one is slightly larger, but the offset is lower so it will not trap. + (memory 1 2 shared) + (data (i32.const 65534) "\00\00") +) + +;; CHECK: (memory $0 1 2 shared) + +;; _TNH_: (memory $0 1 2 shared) +(module + ;; This one's offset is just large enough to trap. (memory 1 2 shared) (data (i32.const 65536) "\00") ) -;; This offset is unknown, so assume the worst. +;; CHECK: (memory $0 1 2 shared) + +;; CHECK: (data $0 (i32.const 65536) "\00") + +;; _TNH_: (memory $0 1 2 shared) (module + ;; This offset is unknown, so assume the worst. + ;; CHECK: (import "a" "b" (global $g i32)) + ;; _TNH_: (import "a" "b" (global $g i32)) (import "a" "b" (global $g i32)) (memory 1 2 shared) (data (global.get $g) "\00") ) +;; CHECK: (memory $0 1 2 shared) + +;; CHECK: (data $0 (global.get $g) "\00") + +;; _TNH_: (memory $0 1 2 shared) + +;; _TNH_: (data $0 (global.get $g) "\00") From 66bf12a431a38d33080fb556c38151f1583673ab Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 31 Jan 2024 11:17:34 -0800 Subject: [PATCH 04/12] work --- test/lit/passes/memory-packing_traps.wast | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/lit/passes/memory-packing_traps.wast b/test/lit/passes/memory-packing_traps.wast index a9308299b3d..ed0a9b75bfc 100644 --- a/test/lit/passes/memory-packing_traps.wast +++ b/test/lit/passes/memory-packing_traps.wast @@ -69,6 +69,8 @@ ;; _TNH_: (memory $0 1 2 shared) (module ;; This offset is unknown, so assume the worst. + ;; TODO: We could remove it in TNH mode + ;; CHECK: (import "a" "b" (global $g i32)) ;; _TNH_: (import "a" "b" (global $g i32)) (import "a" "b" (global $g i32)) From dd924abc0dedf98f64c64fd11da19ad3dd89216c Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 31 Jan 2024 11:40:08 -0800 Subject: [PATCH 05/12] test --- test/lit/passes/memory-packing_traps.wast | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/lit/passes/memory-packing_traps.wast b/test/lit/passes/memory-packing_traps.wast index ed0a9b75bfc..f6b4ec2439e 100644 --- a/test/lit/passes/memory-packing_traps.wast +++ b/test/lit/passes/memory-packing_traps.wast @@ -77,6 +77,7 @@ (memory 1 2 shared) (data (global.get $g) "\00") ) + ;; CHECK: (memory $0 1 2 shared) ;; CHECK: (data $0 (global.get $g) "\00") @@ -84,3 +85,12 @@ ;; _TNH_: (memory $0 1 2 shared) ;; _TNH_: (data $0 (global.get $g) "\00") +(module + ;; Passive segments cannot trap during startup and are removable if they have + ;; no uses, like here. + (memory 1 2 shared) + (data $data "\00\00\00") +) +;; CHECK: (memory $0 1 2 shared) + +;; _TNH_: (memory $0 1 2 shared) From adce58c77010d1108f4e5967994c206a9c8ba884 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 31 Jan 2024 13:03:18 -0800 Subject: [PATCH 06/12] fix test, check prefix cannot start with _ --- test/lit/passes/memory-packing_traps.wast | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/lit/passes/memory-packing_traps.wast b/test/lit/passes/memory-packing_traps.wast index f6b4ec2439e..c9f510ad0e2 100644 --- a/test/lit/passes/memory-packing_traps.wast +++ b/test/lit/passes/memory-packing_traps.wast @@ -1,7 +1,7 @@ ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. ;; RUN: foreach %s %t wasm-opt --memory-packing -all --zero-filled-memory -S -o - | filecheck %s -;; RUN: foreach %s %t wasm-opt --memory-packing -all --zero-filled-memory -tnh -S -o - | filecheck %s --check-prefix=_TNH_ +;; RUN: foreach %s %t wasm-opt --memory-packing -all --zero-filled-memory -tnh -S -o - | filecheck %s --check-prefix=TNH__ (module ;; We should not optimize out a segment that will trap, as that is an effect @@ -14,7 +14,7 @@ ;; CHECK: (data $0 (i32.const -1) "\00") -;; _TNH_: (memory $0 1 2 shared) +;; TNH__: (memory $0 1 2 shared) (module ;; We should handle the possible overflow in adding the offset and size, and ;; see this might trap. @@ -26,7 +26,7 @@ ;; CHECK: (data $0 (i32.const -2) "\00\00\00") -;; _TNH_: (memory $0 1 2 shared) +;; TNH__: (memory $0 1 2 shared) (module ;; This segment will almost trap, but not. (memory 1 2 shared) @@ -35,7 +35,7 @@ ;; CHECK: (memory $0 1 2 shared) -;; _TNH_: (memory $0 1 2 shared) +;; TNH__: (memory $0 1 2 shared) (module ;; This one is slightly larger, and will trap. (memory 1 2 shared) @@ -46,7 +46,7 @@ ;; CHECK: (data $0 (i32.const 65535) "\00\00") -;; _TNH_: (memory $0 1 2 shared) +;; TNH__: (memory $0 1 2 shared) (module ;; This one is slightly larger, but the offset is lower so it will not trap. (memory 1 2 shared) @@ -55,7 +55,7 @@ ;; CHECK: (memory $0 1 2 shared) -;; _TNH_: (memory $0 1 2 shared) +;; TNH__: (memory $0 1 2 shared) (module ;; This one's offset is just large enough to trap. (memory 1 2 shared) @@ -66,13 +66,13 @@ ;; CHECK: (data $0 (i32.const 65536) "\00") -;; _TNH_: (memory $0 1 2 shared) +;; TNH__: (memory $0 1 2 shared) (module ;; This offset is unknown, so assume the worst. ;; TODO: We could remove it in TNH mode ;; CHECK: (import "a" "b" (global $g i32)) - ;; _TNH_: (import "a" "b" (global $g i32)) + ;; TNH__: (import "a" "b" (global $g i32)) (import "a" "b" (global $g i32)) (memory 1 2 shared) (data (global.get $g) "\00") @@ -82,9 +82,9 @@ ;; CHECK: (data $0 (global.get $g) "\00") -;; _TNH_: (memory $0 1 2 shared) +;; TNH__: (memory $0 1 2 shared) -;; _TNH_: (data $0 (global.get $g) "\00") +;; TNH__: (data $0 (global.get $g) "\00") (module ;; Passive segments cannot trap during startup and are removable if they have ;; no uses, like here. @@ -93,4 +93,4 @@ ) ;; CHECK: (memory $0 1 2 shared) -;; _TNH_: (memory $0 1 2 shared) +;; TNH__: (memory $0 1 2 shared) From 784978c02687785e170374c2162ce97e85cd64e6 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 31 Jan 2024 15:56:07 -0800 Subject: [PATCH 07/12] feedback: name test items --- test/lit/passes/memory-packing_traps.wast | 88 ++++++++++------------- 1 file changed, 37 insertions(+), 51 deletions(-) diff --git a/test/lit/passes/memory-packing_traps.wast b/test/lit/passes/memory-packing_traps.wast index c9f510ad0e2..cd8ad60f1b2 100644 --- a/test/lit/passes/memory-packing_traps.wast +++ b/test/lit/passes/memory-packing_traps.wast @@ -6,67 +6,57 @@ (module ;; We should not optimize out a segment that will trap, as that is an effect ;; we need to preserve (unless TrapsNeverHappen). - (memory 1 2 shared) - (data (i32.const -1) "\00") + ;; CHECK: (memory $memory 1 2 shared) + ;; TNH__: (memory $memory 1 2 shared) + (memory $memory 1 2 shared) + ;; CHECK: (data $data (i32.const -1) "\00") + (data $data (i32.const -1) "\00") ) -;; CHECK: (memory $0 1 2 shared) - -;; CHECK: (data $0 (i32.const -1) "\00") - -;; TNH__: (memory $0 1 2 shared) (module ;; We should handle the possible overflow in adding the offset and size, and ;; see this might trap. - (memory 1 2 shared) - (data (i32.const -2) "\00\00\00") + ;; CHECK: (memory $memory 1 2 shared) + ;; TNH__: (memory $memory 1 2 shared) + (memory $memory 1 2 shared) + ;; CHECK: (data $data (i32.const -2) "\00\00\00") + (data $data (i32.const -2) "\00\00\00") ) -;; CHECK: (memory $0 1 2 shared) - -;; CHECK: (data $0 (i32.const -2) "\00\00\00") - -;; TNH__: (memory $0 1 2 shared) (module ;; This segment will almost trap, but not. - (memory 1 2 shared) - (data (i32.const 65535) "\00") + ;; CHECK: (memory $memory 1 2 shared) + ;; TNH__: (memory $memory 1 2 shared) + (memory $memory 1 2 shared) + (data $data (i32.const 65535) "\00") ) -;; CHECK: (memory $0 1 2 shared) - -;; TNH__: (memory $0 1 2 shared) (module ;; This one is slightly larger, and will trap. - (memory 1 2 shared) - (data (i32.const 65535) "\00\00") + ;; CHECK: (memory $memory 1 2 shared) + ;; TNH__: (memory $memory 1 2 shared) + (memory $memory 1 2 shared) + ;; CHECK: (data $data (i32.const 65535) "\00\00") + (data $data (i32.const 65535) "\00\00") ) -;; CHECK: (memory $0 1 2 shared) - -;; CHECK: (data $0 (i32.const 65535) "\00\00") - -;; TNH__: (memory $0 1 2 shared) (module ;; This one is slightly larger, but the offset is lower so it will not trap. - (memory 1 2 shared) - (data (i32.const 65534) "\00\00") + ;; CHECK: (memory $memory 1 2 shared) + ;; TNH__: (memory $memory 1 2 shared) + (memory $memory 1 2 shared) + (data $data (i32.const 65534) "\00\00") ) -;; CHECK: (memory $0 1 2 shared) - -;; TNH__: (memory $0 1 2 shared) (module ;; This one's offset is just large enough to trap. - (memory 1 2 shared) - (data (i32.const 65536) "\00") + ;; CHECK: (memory $memory 1 2 shared) + ;; TNH__: (memory $memory 1 2 shared) + (memory $memory 1 2 shared) + ;; CHECK: (data $data (i32.const 65536) "\00") + (data $data (i32.const 65536) "\00") ) -;; CHECK: (memory $0 1 2 shared) - -;; CHECK: (data $0 (i32.const 65536) "\00") - -;; TNH__: (memory $0 1 2 shared) (module ;; This offset is unknown, so assume the worst. ;; TODO: We could remove it in TNH mode @@ -74,23 +64,19 @@ ;; CHECK: (import "a" "b" (global $g i32)) ;; TNH__: (import "a" "b" (global $g i32)) (import "a" "b" (global $g i32)) - (memory 1 2 shared) - (data (global.get $g) "\00") + ;; CHECK: (memory $memory 1 2 shared) + ;; TNH__: (memory $memory 1 2 shared) + (memory $memory 1 2 shared) + ;; CHECK: (data $data (global.get $g) "\00") + ;; TNH__: (data $data (global.get $g) "\00") + (data $data (global.get $g) "\00") ) -;; CHECK: (memory $0 1 2 shared) - -;; CHECK: (data $0 (global.get $g) "\00") - -;; TNH__: (memory $0 1 2 shared) - -;; TNH__: (data $0 (global.get $g) "\00") (module ;; Passive segments cannot trap during startup and are removable if they have ;; no uses, like here. - (memory 1 2 shared) + ;; CHECK: (memory $memory 1 2 shared) + ;; TNH__: (memory $memory 1 2 shared) + (memory $memory 1 2 shared) (data $data "\00\00\00") ) -;; CHECK: (memory $0 1 2 shared) - -;; TNH__: (memory $0 1 2 shared) From 35ef8837cee61e68cd68bfd90185c6db8a000810 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 31 Jan 2024 16:07:00 -0800 Subject: [PATCH 08/12] cleaner --- test/lit/passes/memory-packing_traps.wast | 48 +++++++++++------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/test/lit/passes/memory-packing_traps.wast b/test/lit/passes/memory-packing_traps.wast index cd8ad60f1b2..f417c52357c 100644 --- a/test/lit/passes/memory-packing_traps.wast +++ b/test/lit/passes/memory-packing_traps.wast @@ -6,9 +6,9 @@ (module ;; We should not optimize out a segment that will trap, as that is an effect ;; we need to preserve (unless TrapsNeverHappen). - ;; CHECK: (memory $memory 1 2 shared) - ;; TNH__: (memory $memory 1 2 shared) - (memory $memory 1 2 shared) + ;; CHECK: (memory $memory 1 2) + ;; TNH__: (memory $memory 1 2) + (memory $memory 1 2) ;; CHECK: (data $data (i32.const -1) "\00") (data $data (i32.const -1) "\00") ) @@ -16,43 +16,43 @@ (module ;; We should handle the possible overflow in adding the offset and size, and ;; see this might trap. - ;; CHECK: (memory $memory 1 2 shared) - ;; TNH__: (memory $memory 1 2 shared) - (memory $memory 1 2 shared) + ;; CHECK: (memory $memory 1 2) + ;; TNH__: (memory $memory 1 2) + (memory $memory 1 2) ;; CHECK: (data $data (i32.const -2) "\00\00\00") (data $data (i32.const -2) "\00\00\00") ) (module ;; This segment will almost trap, but not. - ;; CHECK: (memory $memory 1 2 shared) - ;; TNH__: (memory $memory 1 2 shared) - (memory $memory 1 2 shared) + ;; CHECK: (memory $memory 1 2) + ;; TNH__: (memory $memory 1 2) + (memory $memory 1 2) (data $data (i32.const 65535) "\00") ) (module ;; This one is slightly larger, and will trap. - ;; CHECK: (memory $memory 1 2 shared) - ;; TNH__: (memory $memory 1 2 shared) - (memory $memory 1 2 shared) + ;; CHECK: (memory $memory 1 2) + ;; TNH__: (memory $memory 1 2) + (memory $memory 1 2) ;; CHECK: (data $data (i32.const 65535) "\00\00") (data $data (i32.const 65535) "\00\00") ) (module ;; This one is slightly larger, but the offset is lower so it will not trap. - ;; CHECK: (memory $memory 1 2 shared) - ;; TNH__: (memory $memory 1 2 shared) - (memory $memory 1 2 shared) + ;; CHECK: (memory $memory 1 2) + ;; TNH__: (memory $memory 1 2) + (memory $memory 1 2) (data $data (i32.const 65534) "\00\00") ) (module ;; This one's offset is just large enough to trap. - ;; CHECK: (memory $memory 1 2 shared) - ;; TNH__: (memory $memory 1 2 shared) - (memory $memory 1 2 shared) + ;; CHECK: (memory $memory 1 2) + ;; TNH__: (memory $memory 1 2) + (memory $memory 1 2) ;; CHECK: (data $data (i32.const 65536) "\00") (data $data (i32.const 65536) "\00") ) @@ -64,9 +64,9 @@ ;; CHECK: (import "a" "b" (global $g i32)) ;; TNH__: (import "a" "b" (global $g i32)) (import "a" "b" (global $g i32)) - ;; CHECK: (memory $memory 1 2 shared) - ;; TNH__: (memory $memory 1 2 shared) - (memory $memory 1 2 shared) + ;; CHECK: (memory $memory 1 2) + ;; TNH__: (memory $memory 1 2) + (memory $memory 1 2) ;; CHECK: (data $data (global.get $g) "\00") ;; TNH__: (data $data (global.get $g) "\00") (data $data (global.get $g) "\00") @@ -75,8 +75,8 @@ (module ;; Passive segments cannot trap during startup and are removable if they have ;; no uses, like here. - ;; CHECK: (memory $memory 1 2 shared) - ;; TNH__: (memory $memory 1 2 shared) - (memory $memory 1 2 shared) + ;; CHECK: (memory $memory 1 2) + ;; TNH__: (memory $memory 1 2) + (memory $memory 1 2) (data $data "\00\00\00") ) From 08c442ee97de1350a2c86e763a8baac5c85ee3db Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 1 Feb 2024 11:36:38 -0800 Subject: [PATCH 09/12] change --- src/passes/MemoryPacking.cpp | 70 +++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 25 deletions(-) diff --git a/src/passes/MemoryPacking.cpp b/src/passes/MemoryPacking.cpp index e88230935aa..8dbbecc7ee1 100644 --- a/src/passes/MemoryPacking.cpp +++ b/src/passes/MemoryPacking.cpp @@ -112,7 +112,8 @@ struct MemoryPacking : public Pass { bool canSplit(Module* module, const std::unique_ptr& segment, const Referrers& referrers); - void calculateRanges(const std::unique_ptr& segment, + void calculateRanges(Module* module, + const std::unique_ptr& segment, const Referrers& referrers, std::vector& ranges); void createSplitSegments(Builder& builder, @@ -165,7 +166,7 @@ void MemoryPacking::run(Module* module) { std::vector ranges; if (canSplit(module, segment, currReferrers)) { - calculateRanges(segment, currReferrers, ranges); + calculateRanges(module, segment, currReferrers, ranges); } else { // A single range covers the entire segment. Set isZero to false so the // original memory.init will be used even if segment is all zeroes. @@ -281,24 +282,6 @@ bool MemoryPacking::canSplit(Module* module, return false; } - // Do not split an active segment that might trap. - if (!getPassOptions().trapsNeverHappen && segment->offset) { - auto* c = segment->offset->dynCast(); - if (!c) { - // We can't tell if this will trap or not. - return false; - } - auto* memory = module->getMemory(segment->memory); - auto memorySize = memory->initial * Memory::kPageSize; - Index start = c->value.getUnsigned(); - Index size = segment->data.size(); - Index end; - if (std::ckd_add(&end, start, size) || end > memorySize) { - // This traps. - return false; - } - } - for (auto* referrer : referrers) { if (auto* curr = referrer->dynCast()) { if (segment->isPassive) { @@ -317,7 +300,8 @@ bool MemoryPacking::canSplit(Module* module, return segment->isPassive || segment->offset->is(); } -void MemoryPacking::calculateRanges(const std::unique_ptr& segment, +void MemoryPacking::calculateRanges(Module* module, + const std::unique_ptr& segment, const Referrers& referrers, std::vector& ranges) { auto& data = segment->data; @@ -325,6 +309,24 @@ void MemoryPacking::calculateRanges(const std::unique_ptr& segment, return; } + // A segment that might trap during startup must be handled carefully, as we + // do not want to remove that trap (unless we are allowed to by TNH). + auto preserveTrap = !getPassOptions().trapsNeverHappen && segment->offset; + if (preserveTrap) { + // Check if we can rule out a trap by it being in bounds. + if (auto* c = segment->offset->dynCast()) { + auto* memory = module->getMemory(segment->memory); + auto memorySize = memory->initial * Memory::kPageSize; + Index start = c->value.getUnsigned(); + Index size = segment->data.size(); + Index end; + if (!std::ckd_add(&end, start, size) && end <= memorySize) { + // This is in bounds. + preserveTrap = false; + } + } + } + // Calculate initial zero and nonzero ranges size_t start = 0; while (start < data.size()) { @@ -368,7 +370,10 @@ void MemoryPacking::calculateRanges(const std::unique_ptr& segment, } } - // Merge edge zeroes if they are not worth splitting + // Merge edge zeroes if they are not worth splitting. Note that we must not + // have a trap we must preserve here (if we did, we'd need to handle that in + // the code below). + assert(!preserveTrap); if (ranges.size() >= 2) { auto last = ranges.end() - 1; auto penultimate = ranges.end() - 2; @@ -386,12 +391,12 @@ void MemoryPacking::calculateRanges(const std::unique_ptr& segment, } } } else { - // Legacy ballpark overhead of active segment with offset + // Legacy ballpark overhead of active segment with offset. // TODO: Tune this threshold = 8; } - // Merge ranges across small spans of zeroes + // Merge ranges across small spans of zeroes. std::vector mergedRanges = {ranges.front()}; size_t i; for (i = 1; i < ranges.size() - 1; ++i) { @@ -405,10 +410,25 @@ void MemoryPacking::calculateRanges(const std::unique_ptr& segment, mergedRanges.push_back(*curr); } } - // Add the final range if it hasn't already been merged in + // Add the final range if it hasn't already been merged in. if (i < ranges.size()) { mergedRanges.push_back(ranges.back()); } + // If we need to preserve a trap then we must keep the topmost byte of the + // segment, which is enough to cause a trap if we do in fact trap. + if (preserveTrap) { + // Check if the last byte is in a zero range. Such a range will be dropped + // later, so add a non-zero range with that byte. (It is slightly odd to + // add a range with a zero marked as non-zero, but that is how we ensure it + // is preserved later in the output.) + auto& back = mergedRanges.back(); + if (back.isZero) { + // Note that this might make |back| have size 0, but that is not a problem + // as it will be dropped later anyhow. + back.end--; + mergedRanges.push_back({false, back.end, back.end + 1}); + } + } std::swap(ranges, mergedRanges); } From 0edcf1b96706ba3b943338718f1972111be28848 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 1 Feb 2024 12:52:36 -0800 Subject: [PATCH 10/12] finish --- src/passes/MemoryPacking.cpp | 31 +++++++++++++++++++---- test/lit/passes/memory-packing_traps.wast | 11 +++++--- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/src/passes/MemoryPacking.cpp b/src/passes/MemoryPacking.cpp index 8dbbecc7ee1..ffb16ce492d 100644 --- a/src/passes/MemoryPacking.cpp +++ b/src/passes/MemoryPacking.cpp @@ -423,10 +423,13 @@ void MemoryPacking::calculateRanges(Module* module, // is preserved later in the output.) auto& back = mergedRanges.back(); if (back.isZero) { - // Note that this might make |back| have size 0, but that is not a problem - // as it will be dropped later anyhow. + // Remove the last byte from |back|. Decrementing this prevents it from + // overlapping with the new segment we are about to add. Note that this + // might make |back| have size 0, but that is not a problem as it will be + // dropped later anyhow, since it contains zeroes. back.end--; - mergedRanges.push_back({false, back.end, back.end + 1}); + auto lastByte = data.size() - 1; + mergedRanges.push_back({false, lastByte, lastByte + 1}); } } std::swap(ranges, mergedRanges); @@ -593,6 +596,20 @@ void MemoryPacking::dropUnusedSegments( module->updateDataSegmentsMap(); } +// Given the start of a segment and an offset into it, compute the sum of the +// two to get the absolute address the data should be at. This takes into +// account overflows in that we saturate to UINT_MAX: we do not want an overflow +// to take us down into a small address; in the invalid case of an overflow we +// stay at the largest possible unsigned value, which will keep us trapping. +template +Expression* addStartAndOffset(T start, T offset, Builder& builder) { + T total; + if (std::ckd_add(&total, start, offset)) { + total = std::numeric_limits::max(); + } + return builder.makeConst(T(total)); +} + void MemoryPacking::createSplitSegments( Builder& builder, const DataSegment* segment, @@ -610,10 +627,14 @@ void MemoryPacking::createSplitSegments( if (!segment->isPassive) { if (auto* c = segment->offset->dynCast()) { if (c->value.type == Type::i32) { - offset = builder.makeConst(int32_t(c->value.geti32() + range.start)); + offset = addStartAndOffset(range.start, + c->value.geti32(), + builder); } else { assert(c->value.type == Type::i64); - offset = builder.makeConst(int64_t(c->value.geti64() + range.start)); + offset = addStartAndOffset(range.start, + c->value.geti64(), + builder); } } else { assert(ranges.size() == 1); diff --git a/test/lit/passes/memory-packing_traps.wast b/test/lit/passes/memory-packing_traps.wast index f417c52357c..e7677ff845a 100644 --- a/test/lit/passes/memory-packing_traps.wast +++ b/test/lit/passes/memory-packing_traps.wast @@ -15,11 +15,12 @@ (module ;; We should handle the possible overflow in adding the offset and size, and - ;; see this might trap. + ;; see this might trap. To keep the segment trapping, we will emit a segment + ;; with offset -1 of size 1 (which is the minimal thing we need for a trap). ;; CHECK: (memory $memory 1 2) ;; TNH__: (memory $memory 1 2) (memory $memory 1 2) - ;; CHECK: (data $data (i32.const -2) "\00\00\00") + ;; CHECK: (data $data (i32.const -1) "\00") (data $data (i32.const -2) "\00\00\00") ) @@ -32,11 +33,13 @@ ) (module - ;; This one is slightly larger, and will trap. + ;; This one is slightly larger, and will trap. We can at least shorten the + ;; segment to only contain one byte, at the highest address the segment would + ;; write to. ;; CHECK: (memory $memory 1 2) ;; TNH__: (memory $memory 1 2) (memory $memory 1 2) - ;; CHECK: (data $data (i32.const 65535) "\00\00") + ;; CHECK: (data $data (i32.const 65536) "\00") (data $data (i32.const 65535) "\00\00") ) From 4aec05b3ba1647971eb812990b333b80b0524d53 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 1 Feb 2024 12:52:45 -0800 Subject: [PATCH 11/12] format --- src/passes/MemoryPacking.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/passes/MemoryPacking.cpp b/src/passes/MemoryPacking.cpp index ffb16ce492d..8bf5da94bde 100644 --- a/src/passes/MemoryPacking.cpp +++ b/src/passes/MemoryPacking.cpp @@ -627,14 +627,12 @@ void MemoryPacking::createSplitSegments( if (!segment->isPassive) { if (auto* c = segment->offset->dynCast()) { if (c->value.type == Type::i32) { - offset = addStartAndOffset(range.start, - c->value.geti32(), - builder); + offset = addStartAndOffset( + range.start, c->value.geti32(), builder); } else { assert(c->value.type == Type::i64); - offset = addStartAndOffset(range.start, - c->value.geti64(), - builder); + offset = addStartAndOffset( + range.start, c->value.geti64(), builder); } } else { assert(ranges.size() == 1); From 0a24de71a1e4de53290dd0632e63896e36ce31c1 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 1 Feb 2024 12:53:24 -0800 Subject: [PATCH 12/12] cleanup --- src/passes/MemoryPacking.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/passes/MemoryPacking.cpp b/src/passes/MemoryPacking.cpp index 8bf5da94bde..5ecbc587266 100644 --- a/src/passes/MemoryPacking.cpp +++ b/src/passes/MemoryPacking.cpp @@ -109,8 +109,7 @@ struct MemoryPacking : public Pass { void dropUnusedSegments(Module* module, std::vector>& segments, ReferrersMap& referrers); - bool canSplit(Module* module, - const std::unique_ptr& segment, + bool canSplit(const std::unique_ptr& segment, const Referrers& referrers); void calculateRanges(Module* module, const std::unique_ptr& segment, @@ -165,7 +164,7 @@ void MemoryPacking::run(Module* module) { std::vector ranges; - if (canSplit(module, segment, currReferrers)) { + if (canSplit(segment, currReferrers)) { calculateRanges(module, segment, currReferrers, ranges); } else { // A single range covers the entire segment. Set isZero to false so the @@ -264,8 +263,7 @@ bool MemoryPacking::canOptimize( return true; } -bool MemoryPacking::canSplit(Module* module, - const std::unique_ptr& segment, +bool MemoryPacking::canSplit(const std::unique_ptr& segment, const Referrers& referrers) { // Don't mess with segments related to llvm coverage tools such as // __llvm_covfun. There segments are expected/parsed by external downstream