PR review

Author: jacob-shops

zjit/src/hir.rs

@@ -2594,8 +2594,8 @@ impl Block {
         self.insns.iter()
     }
 
-    pub fn terminator(&self) -> &InsnId {
-        self.insns().last().unwrap()
+    pub fn terminator(&self) -> InsnId {
+        *self.insns().last().unwrap()
     }
 }
 
@@ -6606,7 +6606,7 @@ impl Function {
         // Collect blocks that terminate with Jump or CondBranch instructions that pass at least one block param along.
         let blocks_sending_params: Vec<BlockId> = blocks.iter().copied()
             .filter(|&block_id|
-                self.resolve(*self.blocks[block_id].terminator()).insn(self).outgoing_edges().any(|edge| !edge.args.is_empty()))
+                self.resolve(self.blocks[block_id].terminator()).insn(self).outgoing_edges().any(|edge| !edge.args.is_empty()))
             .collect();
 
         // We only need to update blocks that have params. (Blocks without params cannot be improved)
@@ -6638,7 +6638,7 @@ impl Function {
             for &block_id in &blocks_sending_params {
                 // Use the results of abstract interpretation to update the states
                 // Perform abstract interpretation
-                let edges = self.resolve(*self.blocks[block_id].terminator()).insn(self).outgoing_edges();
+                let edges = self.resolve(self.blocks[block_id].terminator()).insn(self).outgoing_edges();
                 for BranchEdge { target: target_block_id, args: params } in edges {
                     for (i, param) in params.iter().enumerate() {
                         let param = self.find_id(*param);
@@ -6689,7 +6689,7 @@ impl Function {
 
                 // Update the terminators (basic blocks can only branch at the terminator. This is where block params are passed)
                 for jump_block_id in &blocks_sending_params {
-                    let edges = self.resolve(*self.blocks[*jump_block_id].terminator()).insn_mut(self).outgoing_edges_mut();
+                    let edges = self.resolve(self.blocks[*jump_block_id].terminator()).insn_mut(self).outgoing_edges_mut();
                     for edge in edges {
                         if edge.target == *block_id {
                             prune_vec_by_indices(&mut edge.args, &trivial_indices);
@@ -6728,8 +6728,7 @@ impl Function {
         loop {
             for (rpo_index, &block_id) in rpo.iter().enumerate() {
                 let mut block_cache: HashMap<Key, InsnId>  = HashMap::new();
-                // Populate the block cache with information from predecessors
-                // If all predecessors contain the same entry and value, add it to the map
+                // Set block_cache equal to the intersection of cached insns of all predecessors
                 match cfi.predecessors(block_id) {
                     [] => {},
                     [head] => {
@@ -6799,9 +6798,9 @@ impl Function {
                                         continue
                                     }
                                 }
-                                Entry::Vacant(_) => {
+                                Entry::Vacant(entry) => {
                                     // If the value has not been accessed, cache a copy to optimize future loads or stores.
-                                    block_cache.insert(key, insn_id);
+                                    entry.insert(insn_id);
                                 }
                             }
                             insn_id
@@ -6829,7 +6828,7 @@ impl Function {
                 self.blocks[block_id].insns = new_insns;
 
                 // Check for back edges
-                for edge in self.resolve(*self.blocks[block_id].terminator()).insn(self).outgoing_edges() {
+                for edge in self.resolve(self.blocks[block_id].terminator()).insn(self).outgoing_edges() {
                     if rpo_order[edge.target] <= rpo_index {
                         has_back_edge |= true;
                     }

zjit/src/hir/tests.rs

@@ -6808,74 +6808,6 @@ pub(crate) mod hir_build_tests {
           Return v39
         ");
     }
-
-    #[test]
-    fn test_global_load_elimination() {
-        set_call_threshold(3);
-        eval(r#"
-           class TestObj
-             def initialize(value)
-               @value = value
-             end
-
-             def read(val)
-               even = val.even?
-               a = @value
-               even ? a : @value + 1
-             end
-           end
-
-           obj = TestObj.new(5)
-           obj.read(1)
-           obj.read(2)
-        "#);
-
-        assert_snapshot!(
-           hir_string_proc("TestObj.instance_method(:read)"),
-           @"
-        fn read@<compiled>:8:
-        bb1():
-          EntryPoint interpreter
-          v1:BasicObject = LoadSelf
-          v2:CPtr = LoadSP
-          v3:BasicObject = LoadField v2, :val@0x1000
-          v4:NilClass = Const Value(nil)
-          v5:NilClass = Const Value(nil)
-          Jump bb3(v1, v3, v4, v5)
-        bb2():
-          EntryPoint JIT(0)
-          v8:BasicObject = LoadArg :self@0
-          v9:BasicObject = LoadArg :val@1
-          v10:NilClass = Const Value(nil)
-          v11:NilClass = Const Value(nil)
-          Jump bb3(v8, v9, v10, v11)
-        bb3(v13:BasicObject, v14:BasicObject, v15:NilClass, v16:NilClass):
-          v21:BasicObject = Send v14, :even? # SendFallbackReason: Uncategorized(opt_send_without_block)
-          PatchPoint NoEPEscape(read)
-          v27:HeapBasicObject = GuardType v13, HeapBasicObject
-          v28:CShape = LoadField v27, :shape_id@0x1001
-          v29:CShape[0x1002] = GuardBitEquals v28, CShape(0x1002) recompile
-          v30:BasicObject = LoadField v27, :@value@0x1003
-          PatchPoint NoEPEscape(read)
-          v37:CBool = Test v21
-          v38:Falsy = RefineType v21, Falsy
-          CondBranch v37, bb5(), bb4(v13, v14, v38, v30)
-        bb5():
-          v40:Truthy = RefineType v21, Truthy
-          CheckInterrupts
-          Return v30
-        bb4(v46:BasicObject, v47:BasicObject, v48:Falsy, v49:BasicObject):
-          v52:HeapBasicObject = GuardType v46, HeapBasicObject
-          v53:CShape = LoadField v52, :shape_id@0x1001
-          v54:CShape[0x1002] = GuardBitEquals v53, CShape(0x1002) recompile
-          v55:BasicObject = LoadField v52, :@value@0x1003
-          v57:Fixnum[1] = Const Value(1)
-          v60:BasicObject = Send v55, :+, v57 # SendFallbackReason: Uncategorized(opt_plus)
-          CheckInterrupts
-          Return v60
-        "
-        );
-    }
 }
 
  /// Test successor and predecessor set computations.