Clean up for PR review

Author: jacob-shops

zjit/src/hir.rs

@@ -4607,18 +4607,7 @@ impl Function {
         self.infer_types();
     }
 
-    // struct Heap {
-    //     fn load(obj, offset)
-    //     fn kill(obj, offset)
-    //     fn store(obj, offset, val)
-    // }
-
     fn optimize_load_store(&mut self) {
-        // TODO: Add specific tests for load_store
-        // TODO: Add dead store elimination
-        // The key for the hashmap should be type and offset, with a value of value
-        // This lets us to index in with both load and store fields since insn_ids are probably always going to be different and we can't easily match on that
-        // So... how do we match against and store the enum label without all the data?? not sure yet :/
         let mut compile_time_heap: HashMap<(InsnId, i32), InsnId>  = HashMap::new();
         for block in self.rpo() {
             let old_insns = std::mem::take(&mut self.blocks[block.0].insns);
@@ -4644,13 +4633,12 @@ impl Function {
                         let key = (self.chase_insn(recv), offset);
                         match compile_time_heap.entry(key) {
                             std::collections::hash_map::Entry::Occupied(entry) => {
-                                // If the value is already saved, we can't short circuit like we can with a store.
-                                // However, we can avoid the load with a reference to the representative to the union from SSA.
+                                // If the value is stored already, we should short circuit.
+                                // However, we need to replace insn_id with its representative in the SSA union.
                                 self.make_equal_to(insn_id, *entry.get());
                                 continue
                             }
                             std::collections::hash_map::Entry::Vacant(_) => {
-                                // TODO(Jacob): Make sure this is correct, could be wrong?
                                 compile_time_heap.insert(key, insn_id);
                             }
                         }

zjit/src/hir/opt_tests.rs

@@ -13690,7 +13690,7 @@ mod hir_opt_tests {
     }
 
     #[test]
-    fn test_double_store_removal() {
+    fn test_double_store_with_removal() {
         eval("
             class C
               def initialize
@@ -13732,4 +13732,156 @@ mod hir_opt_tests {
           Return v13
         ");
     }
+
+    #[test]
+    fn test_double_store_no_removal_with_alias_between() {
+        eval("
+            class C
+              def initialize
+                a = 1
+                @a = a
+                @b = a
+                @a = a
+              end
+            end
+
+            C.new
+        ");
+        assert_snapshot!(hir_string_proc("C.instance_method(:initialize)"), @r"
+        fn initialize@<compiled>:4:
+        bb1():
+          EntryPoint interpreter
+          v1:BasicObject = LoadSelf
+          v2:NilClass = Const Value(nil)
+          Jump bb3(v1, v2)
+        bb2():
+          EntryPoint JIT(0)
+          v5:BasicObject = LoadArg :self@0
+          v6:NilClass = Const Value(nil)
+          Jump bb3(v5, v6)
+        bb3(v8:BasicObject, v9:NilClass):
+          v13:Fixnum[1] = Const Value(1)
+          PatchPoint SingleRactorMode
+          v43:HeapBasicObject = GuardType v8, HeapBasicObject
+          v44:CShape = LoadField v43, :_shape_id@0x1000
+          v45:CShape[0x1001] = GuardBitEquals v44, CShape(0x1001)
+          StoreField v43, :@a@0x1002, v13
+          WriteBarrier v43, v13
+          v48:CShape[0x1003] = Const CShape(0x1003)
+          StoreField v43, :_shape_id@0x1000, v48
+          v20:HeapBasicObject = RefineType v8, HeapBasicObject
+          PatchPoint NoEPEscape(initialize)
+          PatchPoint SingleRactorMode
+          StoreField v20, :@b@0x1004, v13
+          WriteBarrier v20, v13
+          v55:CShape[0x1005] = Const CShape(0x1005)
+          StoreField v20, :_shape_id@0x1000, v55
+          v28:HeapBasicObject = RefineType v20, HeapBasicObject
+          PatchPoint NoEPEscape(initialize)
+          PatchPoint SingleRactorMode
+          WriteBarrier v28, v13
+          CheckInterrupts
+          Return v13
+        ");
+    }
+
+    #[test]
+    fn test_double_store_with_removal_and_insns_between() {
+        eval("
+            class C
+              def initialize
+                a = 1
+                @a = a
+                b = 5
+                b += a
+                @a = a
+              end
+            end
+
+            C.new
+        ");
+        assert_snapshot!(hir_string_proc("C.instance_method(:initialize)"), @r"
+        fn initialize@<compiled>:4:
+        bb1():
+          EntryPoint interpreter
+          v1:BasicObject = LoadSelf
+          v2:NilClass = Const Value(nil)
+          v3:NilClass = Const Value(nil)
+          Jump bb3(v1, v2, v3)
+        bb2():
+          EntryPoint JIT(0)
+          v6:BasicObject = LoadArg :self@0
+          v7:NilClass = Const Value(nil)
+          v8:NilClass = Const Value(nil)
+          Jump bb3(v6, v7, v8)
+        bb3(v10:BasicObject, v11:NilClass, v12:NilClass):
+          v16:Fixnum[1] = Const Value(1)
+          PatchPoint SingleRactorMode
+          v49:HeapBasicObject = GuardType v10, HeapBasicObject
+          v50:CShape = LoadField v49, :_shape_id@0x1000
+          v51:CShape[0x1001] = GuardBitEquals v50, CShape(0x1001)
+          StoreField v49, :@a@0x1002, v16
+          WriteBarrier v49, v16
+          v54:CShape[0x1003] = Const CShape(0x1003)
+          StoreField v49, :_shape_id@0x1000, v54
+          v23:HeapBasicObject = RefineType v10, HeapBasicObject
+          v26:Fixnum[5] = Const Value(5)
+          PatchPoint NoEPEscape(initialize)
+          PatchPoint MethodRedefined(Integer@0x1008, +@0x1010, cme:0x1018)
+          v65:Fixnum[6] = Const Value(6)
+          IncrCounter inline_cfunc_optimized_send_count
+          PatchPoint SingleRactorMode
+          WriteBarrier v23, v16
+          CheckInterrupts
+          Return v16
+        ");
+    }
+
+    #[test]
+    fn test_triple_store_with_removal() {
+        eval("
+            class C
+              def initialize
+                a = 1
+                @a = a
+                @a = a
+                @a = a
+              end
+            end
+
+            C.new
+        ");
+        assert_snapshot!(hir_string_proc("C.instance_method(:initialize)"), @r"
+        fn initialize@<compiled>:4:
+        bb1():
+          EntryPoint interpreter
+          v1:BasicObject = LoadSelf
+          v2:NilClass = Const Value(nil)
+          Jump bb3(v1, v2)
+        bb2():
+          EntryPoint JIT(0)
+          v5:BasicObject = LoadArg :self@0
+          v6:NilClass = Const Value(nil)
+          Jump bb3(v5, v6)
+        bb3(v8:BasicObject, v9:NilClass):
+          v13:Fixnum[1] = Const Value(1)
+          PatchPoint SingleRactorMode
+          v43:HeapBasicObject = GuardType v8, HeapBasicObject
+          v44:CShape = LoadField v43, :_shape_id@0x1000
+          v45:CShape[0x1001] = GuardBitEquals v44, CShape(0x1001)
+          StoreField v43, :@a@0x1002, v13
+          WriteBarrier v43, v13
+          v48:CShape[0x1003] = Const CShape(0x1003)
+          StoreField v43, :_shape_id@0x1000, v48
+          v20:HeapBasicObject = RefineType v8, HeapBasicObject
+          PatchPoint NoEPEscape(initialize)
+          PatchPoint SingleRactorMode
+          WriteBarrier v20, v13
+          v28:HeapBasicObject = RefineType v20, HeapBasicObject
+          StoreField v28, :@a@0x1002, v13
+          WriteBarrier v28, v13
+          CheckInterrupts
+          Return v13
+        ");
+    }
 }