From 586a8835e9608b0bbaf8d2c62d7039da6fe93dc0 Mon Sep 17 00:00:00 2001 From: Lachlan Sneff Date: Tue, 23 Oct 2018 00:50:09 -0400 Subject: [PATCH] Add a `readonly` flag for loads (#562) * Add readonly MemFlag * Add readonly flag verifier check * Make global loads readonly * Fix gvn to consider readonly loads --- cranelift/docs/ir.rst | 13 +++++----- cranelift/filetests/simple_gvn/readonly.clif | 25 +++++++++++++++++++ lib/codegen/src/ir/globalvalue.rs | 16 ++++++++++-- lib/codegen/src/ir/memflags.rs | 17 ++++++++++++- lib/codegen/src/legalizer/globalvalue.rs | 9 +++++-- lib/codegen/src/simple_gvn.rs | 20 ++++++++++++++- lib/codegen/src/verifier/mod.rs | 26 ++++++++++++++++++++ lib/reader/src/parser.rs | 7 +++--- lib/wasm/src/environ/dummy.rs | 3 +++ 9 files changed, 120 insertions(+), 16 deletions(-) create mode 100644 cranelift/filetests/simple_gvn/readonly.clif diff --git a/cranelift/docs/ir.rst b/cranelift/docs/ir.rst index c452dfb6bc..5925f4e601 100644 --- a/cranelift/docs/ir.rst +++ b/cranelift/docs/ir.rst @@ -502,12 +502,13 @@ Memory operation flags Loads and stores can have flags that loosen their semantics in order to enable optimizations. -======= =========================================== -Flag Description -======= =========================================== -notrap Memory is assumed to be :term:`accessible`. -aligned Trapping allowed for misaligned accesses. -======= =========================================== +======= =========================================== +Flag Description +======= =========================================== +notrap Memory is assumed to be :term:`accessible`. +aligned Trapping allowed for misaligned accesses. +readonly The data at the specified address will not modified between when this function is called and exited. +======= =========================================== When the ``accessible`` flag is set, the behavior is undefined if the memory is not :term:`accessible`. diff --git a/cranelift/filetests/simple_gvn/readonly.clif b/cranelift/filetests/simple_gvn/readonly.clif new file mode 100644 index 0000000000..28eacce61c --- /dev/null +++ b/cranelift/filetests/simple_gvn/readonly.clif @@ -0,0 +1,25 @@ +test simple-gvn + +target x86_64 + +function %eliminate_redundant_global_loads(i32, i64 vmctx) { + gv0 = vmctx + gv1 = load.i64 notrap aligned readonly gv0 + heap0 = static gv1, min 0x1_0000, bound 0x1_0000_0000, guard 0x8000_0000, index_type i32 + +ebb0(v0: i32, v1: i64): + v2 = heap_addr.i64 heap0, v0, 1 + v3 = heap_addr.i64 heap0, v0, 1 + + v4 = iconst.i32 0 + store.i32 notrap aligned v4, v2 + store.i32 notrap aligned v4, v3 + + return +} +; check: v2 = heap_addr.i64 heap0, v0, 1 +; check: v3 -> v2 +; check: v4 = iconst.i32 0 +; check: store notrap aligned v4, v2 +; check: store notrap aligned v4, v2 +; check: return diff --git a/lib/codegen/src/ir/globalvalue.rs b/lib/codegen/src/ir/globalvalue.rs index 47696fe3d1..b46a2ab4e6 100644 --- a/lib/codegen/src/ir/globalvalue.rs +++ b/lib/codegen/src/ir/globalvalue.rs @@ -15,7 +15,8 @@ pub enum GlobalValueData { /// /// The `base` global value is assumed to contain a pointer. This global value is computed /// by loading from memory at that pointer value. The memory must be accessible, and - /// naturally aligned to hold a value of the type. + /// naturally aligned to hold a value of the type. The data at this address is assumed + /// to never change while the current function is executing. Load { /// The base pointer global value. base: GlobalValue, @@ -25,6 +26,9 @@ pub enum GlobalValueData { /// Type of the loaded value. global_type: Type, + + /// Specifies whether the memory that this refers to is readonly, allowing for the elimination of redundant loads. + readonly: bool, }, /// Value is an offset from another global value. @@ -90,7 +94,15 @@ impl fmt::Display for GlobalValueData { base, offset, global_type, - } => write!(f, "load.{} notrap aligned {}{}", global_type, base, offset), + readonly, + } => write!( + f, + "load.{} notrap aligned {}{}{}", + global_type, + if readonly { "readonly " } else { "" }, + base, + offset + ), GlobalValueData::IAddImm { global_type, base, diff --git a/lib/codegen/src/ir/memflags.rs b/lib/codegen/src/ir/memflags.rs index b05cea2913..f459abdb3f 100644 --- a/lib/codegen/src/ir/memflags.rs +++ b/lib/codegen/src/ir/memflags.rs @@ -5,9 +5,10 @@ use std::fmt; enum FlagBit { Notrap, Aligned, + Readonly, } -const NAMES: [&str; 2] = ["notrap", "aligned"]; +const NAMES: [&str; 3] = ["notrap", "aligned", "readonly"]; /// Flags for memory operations like load/store. /// @@ -79,6 +80,20 @@ impl MemFlags { pub fn set_aligned(&mut self) { self.set(FlagBit::Aligned) } + + /// Test if the `readonly` flag is set. + /// + /// Loads with this flag have no memory dependendies. + /// This results in indefined behavior if the dereferenced memory is mutated at any time + /// between when the function is called and when it is exited. + pub fn readonly(self) -> bool { + self.read(FlagBit::Readonly) + } + + /// Set the `readonly` flag. + pub fn set_readonly(&mut self) { + self.set(FlagBit::Readonly) + } } impl fmt::Display for MemFlags { diff --git a/lib/codegen/src/legalizer/globalvalue.rs b/lib/codegen/src/legalizer/globalvalue.rs index 352c03e13b..1807734c53 100644 --- a/lib/codegen/src/legalizer/globalvalue.rs +++ b/lib/codegen/src/legalizer/globalvalue.rs @@ -38,7 +38,8 @@ pub fn expand_global_value( base, offset, global_type, - } => load_addr(inst, func, base, offset, global_type, isa), + readonly, + } => load_addr(inst, func, base, offset, global_type, readonly, isa), ir::GlobalValueData::Symbol { .. } => symbol(inst, func, gv, isa), } } @@ -88,6 +89,7 @@ fn load_addr( base: ir::GlobalValue, offset: ir::immediates::Offset32, global_type: ir::Type, + readonly: bool, isa: &TargetIsa, ) { // We need to load a pointer from the `base` global value, so insert a new `global_value` @@ -107,10 +109,13 @@ fn load_addr( pos.ins().global_value(ptr_ty, base) }; - // Global-value loads are always notrap and aligned. + // Global-value loads are always notrap and aligned. They may be readonly. let mut mflags = ir::MemFlags::new(); mflags.set_notrap(); mflags.set_aligned(); + if readonly { + mflags.set_readonly(); + } // Perform the load. pos.func diff --git a/lib/codegen/src/simple_gvn.rs b/lib/codegen/src/simple_gvn.rs index 79560af674..54d1040e9d 100644 --- a/lib/codegen/src/simple_gvn.rs +++ b/lib/codegen/src/simple_gvn.rs @@ -18,10 +18,19 @@ fn trivially_unsafe_for_gvn(opcode: Opcode) -> bool { || opcode.can_trap() || opcode.other_side_effects() || opcode.can_store() - || opcode.can_load() || opcode.writes_cpu_flags() } +/// Test that, if the specified instruction is a load, it doesn't have the `readonly` memflag. +fn is_load_and_not_readonly(inst_data: &InstructionData) -> bool { + match *inst_data { + InstructionData::Load { flags, .. } | InstructionData::LoadComplex { flags, .. } => { + !flags.readonly() + } + _ => inst_data.opcode().can_load(), + } +} + /// Wrapper around `InstructionData` which implements `Eq` and `Hash` #[derive(Clone)] struct HashKey<'a, 'f: 'a> { @@ -91,14 +100,23 @@ pub fn do_simple_gvn(func: &mut Function, domtree: &mut DominatorTree) { let func = Ref::map(pos.borrow(), |pos| &pos.func); let opcode = func.dfg[inst].opcode(); + if opcode.is_branch() && !opcode.is_terminator() { scope_stack.push(func.layout.next_inst(inst).unwrap()); visible_values.increment_depth(); } + if trivially_unsafe_for_gvn(opcode) { continue; } + let inst_data = func.dfg[inst].clone(); + + // These are split up to separate concerns. + if is_load_and_not_readonly(&inst_data) { + continue; + } + let ctrl_typevar = func.dfg.ctrl_typevar(inst); let key = HashKey { inst: func.dfg[inst].clone(), diff --git a/lib/codegen/src/verifier/mod.rs b/lib/codegen/src/verifier/mod.rs index 215508ce18..1e4fd631d1 100644 --- a/lib/codegen/src/verifier/mod.rs +++ b/lib/codegen/src/verifier/mod.rs @@ -1651,6 +1651,31 @@ impl<'a> Verifier<'a> { Ok(()) } + fn immediate_constraints( + &self, + inst: Inst, + errors: &mut VerifierErrors, + ) -> VerifierStepResult<()> { + let inst_data = &self.func.dfg[inst]; + + // If this is some sort of a store instruction, get the memflags, else, just return. + let memflags = match *inst_data { + ir::InstructionData::Store { flags, .. } + | ir::InstructionData::StoreComplex { flags, .. } => flags, + _ => return Ok(()), + }; + + if memflags.readonly() { + fatal!( + errors, + inst, + "A store instruction cannot have the `readonly` MemFlag" + ) + } else { + Ok(()) + } + } + pub fn run(&self, errors: &mut VerifierErrors) -> VerifierStepResult<()> { self.verify_global_values(errors)?; self.verify_heaps(errors)?; @@ -1663,6 +1688,7 @@ impl<'a> Verifier<'a> { self.instruction_integrity(inst, errors)?; self.typecheck(inst, errors)?; self.verify_encoding(inst, errors)?; + self.immediate_constraints(inst, errors)?; } } diff --git a/lib/reader/src/parser.rs b/lib/reader/src/parser.rs index 8951fb8dab..c129b76e64 100644 --- a/lib/reader/src/parser.rs +++ b/lib/reader/src/parser.rs @@ -1226,16 +1226,15 @@ impl<'a> Parser<'a> { let flags = self.optional_memflags(); let base = self.match_gv("expected global value: gv«n»")?; let offset = self.optional_offset32()?; - let mut expected_flags = MemFlags::new(); - expected_flags.set_notrap(); - expected_flags.set_aligned(); - if flags != expected_flags { + + if !(flags.notrap() && flags.aligned()) { return err!(self.loc, "global-value load must be notrap and aligned"); } GlobalValueData::Load { base, offset, global_type, + readonly: flags.readonly(), } } "iadd_imm" => { diff --git a/lib/wasm/src/environ/dummy.rs b/lib/wasm/src/environ/dummy.rs index 5608abec6d..20e1462f50 100644 --- a/lib/wasm/src/environ/dummy.rs +++ b/lib/wasm/src/environ/dummy.rs @@ -199,6 +199,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ base: addr, offset: Offset32::new(0), global_type: self.pointer_type(), + readonly: true, }); func.create_heap(ir::HeapData { @@ -219,11 +220,13 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ base: vmctx, offset: Offset32::new(0), global_type: self.pointer_type(), + readonly: true, // when tables in wasm become "growable", revisit whether this can be readonly or not. }); let bound_gv = func.create_global_value(ir::GlobalValueData::Load { base: vmctx, offset: Offset32::new(0), global_type: I32, + readonly: true, }); func.create_table(ir::TableData {