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

@@ -502,12 +502,13 @@ Memory operation flags
 Loads and stores can have flags that loosen their semantics in order to enable
 optimizations.
 
-======= ===========================================
+=======  ===========================================
-Flag    Description
+Flag     Description
-======= ===========================================
+=======  ===========================================
-notrap  Memory is assumed to be :term:`accessible`.
+notrap   Memory is assumed to be :term:`accessible`.
-aligned Trapping allowed for misaligned accesses.
+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`.

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

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,

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 {

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

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(),

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

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();
+                if !(flags.notrap() && flags.aligned()) {
-                expected_flags.set_aligned();
-                if flags != expected_flags {
                     return err!(self.loc, "global-value load must be notrap and aligned");
                 }
                 GlobalValueData::Load {
                     base,
                     offset,
                     global_type,
+                    readonly: flags.readonly(),
                 }
             }
             "iadd_imm" => {

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 {