From f4a2d5337a547db84179749fe62528f00d4e7973 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Wed, 4 Jan 2023 22:03:16 -0800 Subject: [PATCH] Cranelift: GVN `uadd_overflow_trap` (#5520) * Switch duplicate loads w/ dynamic memories test to `min_size = 0` This test was accidentally hitting a special case for bounds checks for when we know that `offset + access_size < min_size` and can skip some steps. This commit changes the `min_size` of the memory to zero so that we are forced to do fully general bounds checks. * Cranelift: Mark `uadd_overflow_trap` as okay for GVN Although this improves our test sequence for duplicate loads with dynamic memories, it unfortunately doesn't have any effect on sightglass benchmarks: ``` instantiation :: instructions-retired :: benchmarks/pulldown-cmark/benchmark.wasm No difference in performance. [34448 35607.23 37158] gvn_uadd_overflow_trap.so [34566 35734.05 36585] main.so instantiation :: instructions-retired :: benchmarks/spidermonkey/benchmark.wasm No difference in performance. [44101 60449.62 92712] gvn_uadd_overflow_trap.so [44011 60436.37 92690] main.so instantiation :: instructions-retired :: benchmarks/bz2/benchmark.wasm No difference in performance. [35595 36675.72 38153] gvn_uadd_overflow_trap.so [35440 36670.42 37993] main.so compilation :: instructions-retired :: benchmarks/bz2/benchmark.wasm No difference in performance. [17370195 17405125.62 17471222] gvn_uadd_overflow_trap.so [17369324 17404859.43 17470725] main.so execution :: instructions-retired :: benchmarks/spidermonkey/benchmark.wasm No difference in performance. [7055720520 7055886880.32 7056265930] gvn_uadd_overflow_trap.so [7055719554 7055843809.33 7056193289] main.so compilation :: instructions-retired :: benchmarks/spidermonkey/benchmark.wasm No difference in performance. [683589861 683767276.00 684098366] gvn_uadd_overflow_trap.so [683590024 683767998.02 684097885] main.so execution :: instructions-retired :: benchmarks/pulldown-cmark/benchmark.wasm No difference in performance. [46436883 46437135.10 46437823] gvn_uadd_overflow_trap.so [46436883 46437087.67 46437785] main.so compilation :: instructions-retired :: benchmarks/pulldown-cmark/benchmark.wasm No difference in performance. [126522461 126565812.58 126647044] gvn_uadd_overflow_trap.so [126522176 126565757.75 126647522] main.so execution :: instructions-retired :: benchmarks/bz2/benchmark.wasm No difference in performance. [653010531 653010533.03 653010544] gvn_uadd_overflow_trap.so [653010531 653010533.18 653010537] main.so ``` * cranelift-codegen-meta: Rename `side_effects_okay_for_gvn` to `side_effects_idempotent` * cranelift-filetests: Ensure there is a trailing newline for blessed Wasm tests --- .../codegen/meta/src/cdsl/instructions.rs | 12 +-- cranelift/codegen/meta/src/gen_inst.rs | 6 +- .../codegen/meta/src/shared/instructions.rs | 5 +- cranelift/codegen/src/simple_gvn.rs | 4 +- .../wasm/duplicate-loads-dynamic-memory.wat | 100 +++++++++--------- cranelift/filetests/src/test_wasm.rs | 4 +- 6 files changed, 67 insertions(+), 64 deletions(-) diff --git a/cranelift/codegen/meta/src/cdsl/instructions.rs b/cranelift/codegen/meta/src/cdsl/instructions.rs index cc2c22b3cd..11025921c5 100644 --- a/cranelift/codegen/meta/src/cdsl/instructions.rs +++ b/cranelift/codegen/meta/src/cdsl/instructions.rs @@ -74,7 +74,7 @@ pub(crate) struct InstructionContent { /// Does this instruction have other side effects besides can_* flags? pub other_side_effects: bool, /// Despite having other side effects, is this instruction okay to GVN? - pub side_effects_okay_for_gvn: bool, + pub side_effects_idempotent: bool, } impl InstructionContent { @@ -135,7 +135,7 @@ pub(crate) struct InstructionBuilder { can_store: bool, can_trap: bool, other_side_effects: bool, - side_effects_okay_for_gvn: bool, + side_effects_idempotent: bool, } impl InstructionBuilder { @@ -155,7 +155,7 @@ impl InstructionBuilder { can_store: false, can_trap: false, other_side_effects: false, - side_effects_okay_for_gvn: false, + side_effects_idempotent: false, } } @@ -215,8 +215,8 @@ impl InstructionBuilder { self } - pub fn side_effects_okay_for_gvn(mut self, val: bool) -> Self { - self.side_effects_okay_for_gvn = val; + pub fn side_effects_idempotent(mut self, val: bool) -> Self { + self.side_effects_idempotent = val; self } @@ -268,7 +268,7 @@ impl InstructionBuilder { can_store: self.can_store, can_trap: self.can_trap, other_side_effects: self.other_side_effects, - side_effects_okay_for_gvn: self.side_effects_okay_for_gvn, + side_effects_idempotent: self.side_effects_idempotent, }) } } diff --git a/cranelift/codegen/meta/src/gen_inst.rs b/cranelift/codegen/meta/src/gen_inst.rs index 9c82753f4c..073869dbb7 100644 --- a/cranelift/codegen/meta/src/gen_inst.rs +++ b/cranelift/codegen/meta/src/gen_inst.rs @@ -514,9 +514,9 @@ fn gen_opcodes(all_inst: &AllInstructions, fmt: &mut Formatter) { ); gen_bool_accessor( all_inst, - |inst| inst.side_effects_okay_for_gvn, - "side_effects_okay_for_gvn", - "Despite `self.other_side_effects() == true`, is this instruction okay to GVN?", + |inst| inst.side_effects_idempotent, + "side_effects_idempotent", + "Despite having side effects, is this instruction okay to GVN?", fmt, ) }); diff --git a/cranelift/codegen/meta/src/shared/instructions.rs b/cranelift/codegen/meta/src/shared/instructions.rs index a9bdcc7ac1..9f4246f4ca 100755 --- a/cranelift/codegen/meta/src/shared/instructions.rs +++ b/cranelift/codegen/meta/src/shared/instructions.rs @@ -1392,7 +1392,7 @@ pub(crate) fn define( .other_side_effects(true) // We can de-duplicate spectre selects since the side effect is // idempotent. - .side_effects_okay_for_gvn(true), + .side_effects_idempotent(true), ); let c = &Operand::new("c", Any).with_doc("Controlling value to test"); @@ -1961,7 +1961,8 @@ pub(crate) fn define( ) .operands_in(vec![x, y, code]) .operands_out(vec![a]) - .can_trap(true), + .can_trap(true) + .side_effects_idempotent(true), ); } diff --git a/cranelift/codegen/src/simple_gvn.rs b/cranelift/codegen/src/simple_gvn.rs index 4de6adad6f..6b09ae96b2 100644 --- a/cranelift/codegen/src/simple_gvn.rs +++ b/cranelift/codegen/src/simple_gvn.rs @@ -15,9 +15,9 @@ fn trivially_unsafe_for_gvn(opcode: Opcode) -> bool { || opcode.is_branch() || opcode.is_terminator() || opcode.is_return() - || opcode.can_trap() || opcode.can_store() - || (opcode.other_side_effects() && !opcode.side_effects_okay_for_gvn()) + || (opcode.can_trap() && !opcode.side_effects_idempotent()) + || (opcode.other_side_effects() && !opcode.side_effects_idempotent()) } /// Test that, if the specified instruction is a load, it doesn't have the `readonly` memflag. diff --git a/cranelift/filetests/filetests/wasm/duplicate-loads-dynamic-memory.wat b/cranelift/filetests/filetests/wasm/duplicate-loads-dynamic-memory.wat index 3eca709aef..aab07174d1 100644 --- a/cranelift/filetests/filetests/wasm/duplicate-loads-dynamic-memory.wat +++ b/cranelift/filetests/filetests/wasm/duplicate-loads-dynamic-memory.wat @@ -21,13 +21,13 @@ ;;! ;;! [[heaps]] ;;! base = "heap_base" -;;! min_size = 0x10000 +;;! min_size = 0 ;;! offset_guard_size = 0xffffffff ;;! index_type = "i32" ;;! style = { kind = "dynamic", bound = "heap_bound" } (module - (memory (export "memory") 1) + (memory (export "memory") 0) (func (export "load-without-offset") (param i32) (result i32 i32) local.get 0 i32.load @@ -49,31 +49,31 @@ ;; ;; block0(v0: i32, v1: i64): ;; @0057 v4 = uextend.i64 v0 -;; v13 -> v4 -;; @0057 v5 = load.i64 notrap aligned v1+8 -;; v14 -> v5 -;; v22 = iconst.i64 -4 -;; v23 -> v22 -;; @0057 v6 = iadd v5, v22 ; v22 = -4 -;; v15 -> v6 -;; @0057 v7 = load.i64 notrap aligned v1 -;; v16 -> v7 -;; @0057 v8 = iadd v7, v4 -;; v17 -> v8 -;; @0057 v9 = iconst.i64 0 -;; v18 -> v9 -;; @0057 v10 = icmp ugt v4, v6 -;; v19 -> v10 -;; @0057 v11 = select_spectre_guard v10, v9, v8 ; v9 = 0 -;; v20 -> v11 -;; @0057 v12 = load.i32 little heap v11 -;; v2 -> v12 -;; v21 -> v12 -;; v3 -> v21 +;; v14 -> v4 +;; @0057 v5 = iconst.i64 4 +;; v15 -> v5 +;; @0057 v6 = uadd_overflow_trap v4, v5, heap_oob ; v5 = 4 +;; v16 -> v6 +;; @0057 v7 = load.i64 notrap aligned v1+8 +;; v17 -> v7 +;; @0057 v8 = load.i64 notrap aligned v1 +;; v18 -> v8 +;; @0057 v9 = iadd v8, v4 +;; v19 -> v9 +;; @0057 v10 = iconst.i64 0 +;; v20 -> v10 +;; @0057 v11 = icmp ugt v6, v7 +;; v21 -> v11 +;; @0057 v12 = select_spectre_guard v11, v10, v9 ; v10 = 0 +;; v22 -> v12 +;; @0057 v13 = load.i32 little heap v12 +;; v2 -> v13 +;; v23 -> v13 +;; v3 -> v23 ;; @005f jump block1 ;; ;; block1: -;; @005f return v12, v12 +;; @005f return v13, v13 ;; } ;; ;; function u0:1(i32, i64 vmctx) -> i32, i32 fast { @@ -83,33 +83,33 @@ ;; ;; block0(v0: i32, v1: i64): ;; @0064 v4 = uextend.i64 v0 -;; v14 -> v4 -;; @0064 v5 = load.i64 notrap aligned v1+8 -;; v15 -> v5 -;; v24 = iconst.i64 -1238 -;; v26 -> v24 -;; @0064 v6 = iadd v5, v24 ; v24 = -1238 -;; v16 -> v6 -;; @0064 v7 = load.i64 notrap aligned v1 -;; v17 -> v7 -;; @0064 v8 = iadd v7, v4 -;; v18 -> v8 -;; v25 = iconst.i64 1234 -;; v27 -> v25 -;; @0064 v9 = iadd v8, v25 ; v25 = 1234 -;; v19 -> v9 -;; @0064 v10 = iconst.i64 0 -;; v20 -> v10 -;; @0064 v11 = icmp ugt v4, v6 -;; v21 -> v11 -;; @0064 v12 = select_spectre_guard v11, v10, v9 ; v10 = 0 -;; v22 -> v12 -;; @0064 v13 = load.i32 little heap v12 -;; v2 -> v13 -;; v23 -> v13 -;; v3 -> v23 +;; v15 -> v4 +;; @0064 v5 = iconst.i64 1238 +;; v16 -> v5 +;; @0064 v6 = uadd_overflow_trap v4, v5, heap_oob ; v5 = 1238 +;; v17 -> v6 +;; @0064 v7 = load.i64 notrap aligned v1+8 +;; v18 -> v7 +;; @0064 v8 = load.i64 notrap aligned v1 +;; v19 -> v8 +;; @0064 v9 = iadd v8, v4 +;; v20 -> v9 +;; v26 = iconst.i64 1234 +;; v27 -> v26 +;; @0064 v10 = iadd v9, v26 ; v26 = 1234 +;; v21 -> v10 +;; @0064 v11 = iconst.i64 0 +;; v22 -> v11 +;; @0064 v12 = icmp ugt v6, v7 +;; v23 -> v12 +;; @0064 v13 = select_spectre_guard v12, v11, v10 ; v11 = 0 +;; v24 -> v13 +;; @0064 v14 = load.i32 little heap v13 +;; v2 -> v14 +;; v25 -> v14 +;; v3 -> v25 ;; @006e jump block1 ;; ;; block1: -;; @006e return v13, v13 +;; @006e return v14, v14 ;; } \ No newline at end of file diff --git a/cranelift/filetests/src/test_wasm.rs b/cranelift/filetests/src/test_wasm.rs index 7e75e8f209..45665653a9 100644 --- a/cranelift/filetests/src/test_wasm.rs +++ b/cranelift/filetests/src/test_wasm.rs @@ -114,7 +114,9 @@ pub fn run(path: &Path, wat: &str) -> Result<()> { } })) .collect(); - std::fs::write(path, new_wat_lines.join("\n")) + let mut new_wat = new_wat_lines.join("\n"); + new_wat.push('\n'); + std::fs::write(path, new_wat) .with_context(|| format!("failed to write file: {}", path.display()))?; return Ok(()); }