Add symbolic expression by pfultz2 · Pull Request #4697 · ROCm/AMDMIGraphX

pfultz2 · 2026-03-23T17:33:44Z

Motivation

Technical Details

Changelog Category

Add a CHANGELOG.md entry for any option other than Not Applicable

- Added: New functionality.
- Changed: Changes to existing functionality.
- Removed: Functionality or support that has been removed. (Compared to a previous release)
- Optimized: Component performance that has been optimized or improved.
- Resolved Issues: Known issues from a previous version that have been resolved.
- Not Applicable: This PR is not to be included in the changelog.

Copilot

Pull request overview

This PR introduces a new migraphx::sym API for building and evaluating symbolic expressions, including interval evaluation and stringification, and adds comprehensive unit tests for the new functionality.

Changes:

Added migraphx::sym::expr with literal/variable/operator nodes, plus evaluation (eval, eval_interval) and to_string.
Implemented interval arithmetic and interval math helpers (e.g., sin/cos/sqrt/abs).
Added a new test/sym.cpp covering value evaluation, interval evaluation, comparisons, compound assignment, equality, and to_string.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File	Description
`src/include/migraphx/sym.hpp`	Declares the symbolic expression API, node types, interval/value helpers, and the `call`/`call_op` builder utilities.
`src/sym.cpp`	Implements interval ops, expression construction helpers, expression evaluation, and stringification.
`test/sym.cpp`	Adds a broad unit test suite exercising the new symbolic/interval features.
`src/CMakeLists.txt`	Wires `sym.cpp` into the `migraphx` library build.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-23T17:39:59Z

src/sym.cpp

+interval expr::eval_interval(const std::unordered_map<std::string, interval>& vars) const
+{
+    if(auto* n = std::get_if<literal_node>(&pimpl->node))
+        return {n->val, n->val};
+    if(auto* n = std::get_if<variable_node>(&pimpl->node))
+    {


expr::eval_interval() dereferences pimpl unconditionally, so evaluating intervals on a default-constructed/empty expr will null-deref. Please add the same empty-expression handling as suggested for eval()/to_string() (throw or define a safe default).

Copilot · 2026-03-23T17:39:59Z

src/include/migraphx/sym.hpp

+template <class Eval, class EvalInterval>
+expr call_op(std::string name, Eval eval, EvalInterval eval_interval, std::vector<expr> args)
+{
+    static const op_def op{std::move(name), std::move(eval), std::move(eval_interval)};


call_op(std::string, Eval, EvalInterval, ...) uses a static const op_def initialized with std::move(name)/std::move(eval). This makes the first call’s name and functors “stick” for the entire process for a given template instantiation; subsequent calls with the same Eval/EvalInterval types (e.g., same lambda type but different captures or different runtime name) will silently reuse the old op, producing incorrect to_string()/evaluation behavior. Consider removing the static (store the op_def in the expr node, e.g., via shared ownership) or keying a cache by name + callable identity rather than template type alone.

Suggested change

static const op_def op{std::move(name), std::move(eval), std::move(eval_interval)};

op_def op{name, eval, eval_interval};

Copilot · 2026-03-23T17:39:59Z

src/sym.cpp

+value expr::eval(const std::unordered_map<std::string, value>& vars) const
+{
+    if(auto* n = std::get_if<literal_node>(&pimpl->node))
+        return n->val;
+    if(auto* n = std::get_if<variable_node>(&pimpl->node))
+        return vars.at(n->name);
+    auto* n = std::get_if<op_node>(&pimpl->node);


expr::eval() dereferences pimpl unconditionally. Since expr is default-constructible (and tests rely on expr a; being valid for equality), calling eval() on a default-constructed expr will crash. Please add a null check (e.g., throw via MIGRAPHX_THROW with a clear message) or define/avoid default-constructed evaluation semantics.

Copilot · 2026-03-23T17:40:00Z

src/sym.cpp

+std::string expr::to_string() const
+{
+    if(auto* n = std::get_if<literal_node>(&pimpl->node))
+        return value_to_string(n->val);
+    if(auto* n = std::get_if<variable_node>(&pimpl->node))
+        return n->name;
+    auto* n = std::get_if<op_node>(&pimpl->node);
+    if(n->op->name == "neg" and pimpl->children.size() == 1)


expr::to_string() also dereferences pimpl without checking for a default-constructed/empty expression, which can lead to a null dereference. Mirror whatever handling you add for eval() (e.g., MIGRAPHX_THROW for empty expressions) so to_string() is safe on all valid expr objects.

Copilot · 2026-03-23T17:40:00Z

src/sym.cpp

+    if(auto* n = std::get_if<literal_node>(&pimpl->node))
+        return n->val;
+    if(auto* n = std::get_if<variable_node>(&pimpl->node))
+        return vars.at(n->name);


In expr::eval(), variable lookup uses vars.at(n->name), which throws std::out_of_range (and loses the context/message style used elsewhere, e.g., eval_interval() uses MIGRAPHX_THROW). Consider switching to find() + MIGRAPHX_THROW("Variable '...' not found in value map") for consistent error reporting and easier debugging.

Suggested change

return vars.at(n->name);

{

auto it = vars.find(n->name);

if(it != vars.end())

return it->second;

MIGRAPHX_THROW("Variable '" + n->name + "' not found in value map");

}

Copilot · 2026-03-23T17:40:00Z

src/sym.cpp

+    return {rmin, rmax};
+}
+
+interval tan(interval x) { return {std::tan(to<double>(x.min)), std::tan(to<double>(x.max))}; }


tan(interval) currently returns tan(min) and tan(max) only. Unlike sin/cos, tan is not bounded or monotone across intervals that cross an odd multiple of π/2, so this can produce an interval that is not a safe enclosure (or even inverted). Consider detecting when [min,max] crosses a tan asymptote and either returning an unbounded interval (e.g., [-inf, inf] as doubles) or throwing.

Copilot · 2026-03-23T17:40:00Z

test/sym.cpp

+
+TEST_CASE(interval_div_assign)
+{
+    // [10.0,20.0] /= [2.0,5.0] = [2.0,10.0]


The comment in interval_div_assign doesn’t match the actual operands/expected result: the code divides [2,10] by [1,5] and expects [0.4,10], but the comment describes a different example ([10,20]/[2,5]=[2,10]). Please update the comment to reflect the actual test case to avoid confusion.

Suggested change

// [10.0,20.0] /= [2.0,5.0] = [2.0,10.0]

// [2.0,10.0] /= [1.0,5.0] = [0.4,10.0]

codecov · 2026-03-23T21:07:35Z

Codecov Report

❌ Patch coverage is 97.89916% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/sym.cpp	97.95%	4 Missing ⚠️
src/include/migraphx/sym.hpp	97.67%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4697      +/-   ##
===========================================
+ Coverage    92.27%   92.32%   +0.05%     
===========================================
  Files          578      580       +2     
  Lines        28541    28779     +238     
===========================================
+ Hits         26335    26569     +234     
- Misses        2206     2210       +4

Files with missing lines	Coverage Δ
src/include/migraphx/sym.hpp	`97.67% <97.67%> (ø)`
src/sym.cpp	`97.95% <97.95%> (ø)`

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pfultz2 added 13 commits March 20, 2026 17:11

Add sym::expr

45fa74f

Format

3150f5a

Move constructor inline

e1f4252

Format

3e0d84b

Move .cpp

8752a2f

Format

31ca343

Add comparison ops

a6552c1

Format

e84dd47

Add assign operators

c2f3514

Add more functions

0ba12f2

Format

684d536

Add to_string method

90d30ac

Format

cecb90a

Copilot AI review requested due to automatic review settings March 23, 2026 17:33

pfultz2 requested a review from causten as a code owner March 23, 2026 17:33

pfultz2 marked this pull request as draft March 23, 2026 17:34

Copilot started reviewing on behalf of pfultz2 March 23, 2026 17:35 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add symbolic expression#4697

Add symbolic expression#4697
pfultz2 wants to merge 13 commits intodevelopfrom
sym-expr

pfultz2 commented Mar 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

codecov bot commented Mar 23, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	static const op_def op{std::move(name), std::move(eval), std::move(eval_interval)};
	op_def op{name, eval, eval_interval};

-        return vars.at(n->name);
+    {
+        auto it = vars.find(n->name);
+        if(it != vars.end())
+            return it->second;
+        MIGRAPHX_THROW("Variable '" + n->name + "' not found in value map");
+    }

	// [10.0,20.0] /= [2.0,5.0] = [2.0,10.0]
	// [2.0,10.0] /= [1.0,5.0] = [0.4,10.0]

Conversation

pfultz2 commented Mar 23, 2026

Motivation

Technical Details

Changelog Category

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Mar 23, 2026 •

edited

Loading