T0b1/wasmtime
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Cranelift: GVN uadd_overflow_trap (#5520)

Browse Source 
* 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
This commit is contained in:
Nick Fitzgerald
2023-01-04 22:03:16 -08:00
committed by GitHub
parent b46ad1b54d
commit f4a2d5337a
6 changed files with 67 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
cranelift/codegen/meta/src/cdsl/instructions.rs
View File

@@ -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,
})
}
}

6
cranelift/codegen/meta/src/gen_inst.rs
View File

@@ -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,
)
});

5
cranelift/codegen/meta/src/shared/instructions.rs
View File

@@ -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),
);
}

4
cranelift/codegen/src/simple_gvn.rs
View File

@@ -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.

100
cranelift/filetests/filetests/wasm/duplicate-loads-dynamic-memory.wat
View File

@@ -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
;; }

4
cranelift/filetests/src/test_wasm.rs
View File

@@ -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(());
}