ZJIT: Don't fold StringEqual through an always-failing guard

Author: k0kubun

zjit/src/codegen_tests.rs

@@ -3616,6 +3616,29 @@ fn test_opt_eq_string_distinct_objects() {
     assert_contains_opcode("test", YARVINSN_opt_eq);
 }
 
+#[test]
+fn test_opt_eq_string_guard_fails_after_inlining() {
+    eval(r#"
+        # frozen_string_literal: true
+        class Foo
+          def self.bar(l, r) = l == r
+        end
+        def test(flag)
+          foo = Foo
+          if flag
+            foo.bar("a", "b")
+          else
+            foo.bar("a", :sym)
+          end
+        end
+    "#);
+    assert_snapshot!(inspect(r#"
+        test(true) # profile opt_eq in bar
+        test(true) # compile test, inlining bar with a Symbol argument on the untaken branch
+        [test(true), test(false)]
+    "#), @"[false, false]");
+}
+
 #[test]
 fn test_opt_eqq_string_same_operand() {
     assert_snapshot!(inspect(r#"

zjit/src/hir.rs

@@ -6770,20 +6770,24 @@ impl Function {
                         }
                     }
                     &Insn::StringEqual { left, right } => {
-                        let left = self.chase_insn(left);
-                        let right = self.chase_insn(right);
                         // If both operands resolve to the same SSA value,
                         // String#== is guaranteed to be true.
-                        if left == right {
+                        if self.chase_insn(left) == self.chase_insn(right) {
                             self.new_insn(Insn::Const { val: Const::Value(Qtrue) })
                         } else {
+                            // Look at the operand types as StringEqual sees them, without chasing through guards.
+                            // After inlining, a known non-String can flow into a `GuardType String` that always
+                            // fails at run-time, so we must not fold using the pre-guard value.
                             let left_type = self.type_of(left);
                             let right_type = self.type_of(right);
                             match (left_type.ruby_object(), right_type.ruby_object()) {
                                 (Some(left_obj), Some(right_obj))
-                                    if left_obj.is_frozen() && right_obj.is_frozen() =>
+                                    if left_type.is_subtype(types::String)
+                                        && right_type.is_subtype(types::String)
+                                        && left_obj.is_frozen()
+                                        && right_obj.is_frozen() =>
                                 {
-                                    // For known frozen objects, evaluate String#== at compile time.
+                                    // For known frozen Strings, evaluate String#== at compile time.
                                     let val = unsafe { rb_yarv_str_eql_internal(left_obj, right_obj) };
                                     self.new_insn(Insn::Const { val: Const::Value(val) })
                                 }

zjit/src/hir/opt_tests.rs

@@ -16461,6 +16461,72 @@ mod hir_opt_tests {
         ");
     }
 
+    #[test]
+    fn test_not_fold_string_equal_through_failing_guard() {
+        eval(r#"
+            # frozen_string_literal: true
+            class Foo
+              def self.bar(l, r) = l == r
+            end
+            def test(flag)
+              foo = Foo
+              if flag
+                foo.bar("a", "b")
+              else
+                foo.bar("a", :sym)
+              end
+            end
+            test(true)
+            test(true)
+        "#);
+        assert_snapshot!(hir_string("test"), @"
+        fn test@<compiled>:7:
+        bb1():
+          EntryPoint interpreter
+          v1:BasicObject = LoadSelf
+          v2:CPtr = LoadSP
+          v3:BasicObject = LoadField v2, :flag@0x1000
+          Jump bb3(v1, v3)
+        bb2():
+          EntryPoint JIT(0)
+          v7:BasicObject = LoadArg :self@0
+          v8:BasicObject = LoadArg :flag@1
+          Jump bb3(v7, v8)
+        bb3(v11:BasicObject, v12:BasicObject):
+          v101:NilClass = Const Value(nil)
+          PatchPoint StableConstantNames(0x1008, Foo)
+          v18:ClassSubclass[Foo@0x1010] = Const Value(VALUE(0x1010))
+          PatchPoint NoEPEscape(test)
+          v25:CBool = Test v12
+          v26:Falsy = RefineType v12, Falsy
+          CondBranch v25, bb5(), bb4()
+        bb5():
+          v28:Truthy = RefineType v12, Truthy
+          v32:StringExact[VALUE(0x1018)] = Const Value(VALUE(0x1018))
+          v34:StringExact[VALUE(0x1020)] = Const Value(VALUE(0x1020))
+          PatchPoint MethodRedefined(Class@0x1028, bar@0x1030, cme:0x1038)
+          PushInlineFrame :bar, v18 (0x1060), num_args=2
+          PatchPoint NoSingletonClass(String@0x1080)
+          PatchPoint MethodRedefined(String@0x1080, ==@0x1088, cme:0x1090)
+          v113:FalseClass = Const Value(false)
+          CheckInterrupts
+          PopInlineFrame
+          Return v113
+        bb4():
+          v48:StringExact[VALUE(0x1018)] = Const Value(VALUE(0x1018))
+          v50:StaticSymbol[:sym] = Const Value(VALUE(0x10b8))
+          PatchPoint MethodRedefined(Class@0x1028, bar@0x1030, cme:0x1038)
+          PushInlineFrame :bar, v18 (0x1060), num_args=2
+          PatchPoint NoSingletonClass(String@0x1080)
+          PatchPoint MethodRedefined(String@0x1080, ==@0x1088, cme:0x1090)
+          v111 = GuardType v50, String
+          v112:BoolExact = StringEqual v48, v111
+          CheckInterrupts
+          PopInlineFrame
+          Return v112
+        ");
+    }
+
     #[test]
     fn opt_neq_string_nil_falls_back_to_basic_object_neq() {
         eval(r#"