Merge pull request #29937 from pranayr710:fix/texpr-named-value-retired-slot

core: keep named texpr values alive across slot-retiring optimizations

modules/core/src/arithm_expr.cpp

@@ -574,7 +574,7 @@ int TExpr::emitUnary(TOp op, int a, int rdepth, const Scalar& params)
         // "don't notice" the difference and hand out the useful semantics. The sub is necessarily
         // the last instruction and its result the last temp (abs is emitted right after its
         // argument) - retire both, the moveToOutput manoeuvre.
-        if (!prog.empty() && arginfo[a].kind == TEMP &&
+        if (!prog.empty() && arginfo[a].kind == TEMP && !arginfo[a].pinned &&
             prog.back().op == OP_SUB && prog.back().result == a &&
             arginfo[a].index == ntemps - 1)
         {
@@ -695,7 +695,7 @@ int TExpr::moveToOutput(int temp, int out)
         if (ins.result == temp) producer = i;
         if (ins.arg0 == temp || ins.arg1 == temp || ins.arg2 == temp) usedAsArg = true;
     }
-    if (arginfo[temp].kind == TEMP && producer >= 0 && !usedAsArg &&
+    if (arginfo[temp].kind == TEMP && producer >= 0 && !usedAsArg && !arginfo[temp].pinned &&
         arginfo[temp].depth == arginfo[out].depth)
     {
         // MOVE semantics: redirect `temp`'s single producer to write `out` directly, then leave the
@@ -1778,7 +1778,12 @@ struct Parser
                 if (cur.type == T_ASSIGN)
                 {
                     advance();
-                    env[name] = parseTernary();
+                    const int slot = parseTernary();
+                    // The name can be used any number of times below (including not at all), so the
+                    // value must outlive the expression that produced it: pin the slot so the
+                    // retire-the-temp optimizations skip it.
+                    e.arginfo[slot].pinned = true;
+                    env[name] = slot;
                     expect(T_SEMI, "expected ';' after assignment");
                     continue;
                 }

modules/core/src/arithm_expr.hpp

@@ -236,6 +236,12 @@ struct CV_EXPORTS TExpr
         // units. A per-channel scalar of any width is carried this way (no 4-channel Scalar limit).
         int srcdepth = EW_DEPTH_NONE;
         size_t constofs = 0;
+        // Set on a value the parser bound to a name ("t = ...;"). Such a value may be referenced
+        // any number of times later, so the two retire-the-slot manoeuvres (moveToOutput's MOVE and
+        // emitUnary's abs(x-y) peephole) must leave it alone: they reclassify the slot to NONE,
+        // which a later reference would then read as the reserved empty operand. Both take their
+        // non-destructive path (a copy / the plain absdiff form) when this is set.
+        bool pinned = false;
     };
 
     // One compiled instruction: the op + arg-table indices + a resolved kernel (TKernel: fptr +

modules/core/test/test_arithm_expr.cpp

@@ -509,4 +509,68 @@ TEST(Core_TExpr, select_float_mask)
     EXPECT_EQ(0, cvtest::norm(got, exp, NORM_INF));
 }
 
+
+// A value bound to a name may be referenced again after the expression that produced it has been
+// folded away. Both retire-the-slot optimizations used to reclassify that slot to NONE, so the
+// later reference silently read the reserved empty operand instead of the value.
+TEST(Core_TExpr, named_value_reused_after_abs_peephole)
+{
+    Mat a(12, 15, CV_32F), b(12, 15, CV_32F);
+    theRNG().fill(a, RNG::UNIFORM, 1.f, 10.f);
+    theRNG().fill(b, RNG::UNIFORM, 1.f, 10.f);
+
+    // abs(t) folds the subtraction into absdiff; 't' is still live afterwards.
+    Mat got = expr1("t = {0} - {1}; abs(t) + t", { a, b });
+    Mat adiff, diff; cv::absdiff(a, b, adiff); cv::subtract(a, b, diff);
+    Mat exp; cv::add(adiff, diff, exp);
+    EXPECT_LE(cvtest::norm(got, exp, NORM_INF), 1e-3);
+}
+
+TEST(Core_TExpr, named_value_reused_in_tuple_after_abs_peephole)
+{
+    Mat a(9, 11, CV_32F), b(9, 11, CV_32F);
+    theRNG().fill(a, RNG::UNIFORM, 1.f, 10.f);
+    theRNG().fill(b, RNG::UNIFORM, 1.f, 10.f);
+
+    std::vector<Mat> out;
+    cv::texpr("t = {0} - {1}; (abs(t), t)", std::vector<Mat>{ a, b }, out);
+    ASSERT_EQ(out.size(), 2u);
+    Mat adiff, diff; cv::absdiff(a, b, adiff); cv::subtract(a, b, diff);
+    EXPECT_LE(cvtest::norm(out[0], adiff, NORM_INF), 1e-3) << "abs(t)";
+    EXPECT_LE(cvtest::norm(out[1], diff,  NORM_INF), 1e-3) << "t";
+}
+
+// The same slot feeding two outputs: the first output used to MOVE (retire) the named slot,
+// leaving the second reading an empty operand.
+TEST(Core_TExpr, named_value_used_by_two_outputs)
+{
+    Mat a(13, 8, CV_32F), b(13, 8, CV_32F);
+    theRNG().fill(a, RNG::UNIFORM, 1.f, 10.f);
+    theRNG().fill(b, RNG::UNIFORM, 1.f, 10.f);
+
+    std::vector<Mat> out;
+    cv::texpr("t = {0} + {1}; (t, t)", std::vector<Mat>{ a, b }, out);
+    ASSERT_EQ(out.size(), 2u);
+    Mat exp; cv::add(a, b, exp);
+    EXPECT_LE(cvtest::norm(out[0], exp, NORM_INF), 1e-3) << "first";
+    EXPECT_LE(cvtest::norm(out[1], exp, NORM_INF), 1e-3) << "second";
+}
+
+// A named value that is NOT reused must still take the cheap paths (this is the case the
+// retire-the-slot optimizations exist for) - guard against fixing the bug by disabling them.
+TEST(Core_TExpr, named_value_single_use_still_correct)
+{
+    Mat a(10, 10, CV_32F), b(10, 10, CV_32F);
+    theRNG().fill(a, RNG::UNIFORM, 1.f, 10.f);
+    theRNG().fill(b, RNG::UNIFORM, 1.f, 10.f);
+
+    Mat got = expr1("t = {0} - {1}; abs(t)", { a, b });
+    Mat exp; cv::absdiff(a, b, exp);
+    EXPECT_LE(cvtest::norm(got, exp, NORM_INF), 1e-3);
+
+    Mat got2 = expr1("t = {0} + {1}; t", { a, b });
+    Mat exp2; cv::add(a, b, exp2);
+    EXPECT_LE(cvtest::norm(got2, exp2, NORM_INF), 1e-3);
+}
+
 }} // namespace