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