From 78ed76f0d917a4c5c783f7bab084e73c4d38a63f Mon Sep 17 00:00:00 2001 From: pranayr710 Date: Sun, 13 Sep 2026 02:30:55 +0530 Subject: [PATCH] core: keep named texpr values alive across slot-retiring optimizations TExpr::moveToOutput() and the abs(x - y) -> absdiff(x, y) peephole in TExpr::emitUnary() retire a value's arg slot (reclassify it to NONE) once its single consumer has been emitted. That holds for an anonymous intermediate, but a value the parser bound to a name ("t = ...;") may be referenced again: the parser's name table still points at the retired slot, so the later reference resolved to the reserved empty operand and cv::texpr() silently returned a wrong result - for t = {0} - {1}; abs(t) + t t = {0} - {1}; (abs(t), t) t = {0} + {1}; (t, t) the reused name yielded input {0} instead of its own value, with no assertion. Mark a slot as pinned when the parser binds it to a name and skip both retire manoeuvres for a pinned slot; each then takes the non-destructive path it already has - moveToOutput() copies into the output via OP_CAST, and the abs peephole falls through to the plain absdiff(a, 0) form, keeping the OP_SUB that the name still needs. Anonymous intermediates are unaffected, so the zero-temp fast path for single-op programs still fires. --- modules/core/src/arithm_expr.cpp | 11 +++-- modules/core/src/arithm_expr.hpp | 6 +++ modules/core/test/test_arithm_expr.cpp | 64 ++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 3 deletions(-) diff --git a/modules/core/src/arithm_expr.cpp b/modules/core/src/arithm_expr.cpp index 6949b42181..278b21ba17 100644 --- a/modules/core/src/arithm_expr.cpp +++ b/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; } diff --git a/modules/core/src/arithm_expr.hpp b/modules/core/src/arithm_expr.hpp index d0c25d5fc9..9edff364cc 100644 --- a/modules/core/src/arithm_expr.hpp +++ b/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 + diff --git a/modules/core/test/test_arithm_expr.cpp b/modules/core/test/test_arithm_expr.cpp index 26ce8810c9..ec2c0d334d 100644 --- a/modules/core/test/test_arithm_expr.cpp +++ b/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 out; + cv::texpr("t = {0} - {1}; (abs(t), t)", std::vector{ 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 out; + cv::texpr("t = {0} + {1}; (t, t)", std::vector{ 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