Enhance symbolic onnx parsing via sym_eval - #5148
Conversation
Bring the PR #5112-based parser prerequisite branch onto the current development baseline before applying its remaining changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the merged resolver regression compatible with the dynamic-slice interface introduced by PR #5112. Co-authored-by: Cursor <cursoragent@cursor.com>
Track exact integral shape values through parser operations so dynamic consumers retain symbolic output relationships without changing runtime dataflow. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the symbolic-value change focused by restoring existing resolver diagnostics and simplifying the signed Gather size declaration. Co-authored-by: Cursor <cursoragent@cursor.com>
| if(output_shape.type() != shape::int64_type) | ||
| return std::nullopt; | ||
| return compute_symbolic_binary( | ||
| output_shape, input_values, [](const auto& x, const auto& y) { return x + y; }); |
There was a problem hiding this comment.
This shouldn't be added to the add operator. This should be added to the binary class and call the apply method in the derived. Same for unary.
| if(input_values.size() != 2 or not input_values[0].has_value() or | ||
| not input_values[1].has_value()) | ||
| return std::nullopt; | ||
| auto result = broadcast_symbolic_values(*input_values[0], *input_values[1], std::move(f)); |
There was a problem hiding this comment.
I think this makes it more complicated having to reimplement broadcasting(and this will only work on pointwise ops).
Thinking about it more, I think we should keep the shape, and use tensor_view<sym::expr> to iterate the data. We can have a sym_argument class like this:
struct sym_argument
{
bool empty() const
{
return m_data.empty();
}
shape get_shape() const
{
return m_shape;
}
tensor_view<sym::expr> get()
{
return make_view(m_shape, m_data.data());
}
tensor_view<const sym::expr> get() const
{
return make_view(m_shape, m_data.data());
}
std::vector<sym::expr> m_data;
shape m_shape;
};| { | ||
| return [](auto x, auto y) { return x + y; }; | ||
| } | ||
| std::optional<symbolic_tensor_value> |
There was a problem hiding this comment.
I dont think we need to wrap it on an std::optional as empty() also means no value which we can use.
|
|
||
| std::optional<symbolic_tensor_value> result; | ||
| const auto& output_shape = ins.get_shape(); | ||
| if(shape::is_integral(output_shape.type()) and not output_shape.dynamic()) |
There was a problem hiding this comment.
Symbolics support floats so there is no reason to limit it to integrals.
| return; | ||
| } | ||
| transform(input, std::back_inserter(result), [](auto x) { return sym::lit(x); }); | ||
| converted = true; |
There was a problem hiding this comment.
This flag is not needed as you can just check if the vector is empty.
| if constexpr(std::is_unsigned<type>{} and sizeof(type) >= sizeof(int64_t)) | ||
| { | ||
| if(any_of(input, [](auto x) { | ||
| return x > static_cast<type>(std::numeric_limits<int64_t>::max()); |
There was a problem hiding this comment.
We should just clip instead of returning none.
| if(ins.name() == "@literal") | ||
| { | ||
| result = lift_symbolic_tensor_value(ins.get_literal().get_argument()); | ||
| } |
There was a problem hiding this comment.
This should const fold the input if its possible:
| if(ins.name() == "@literal") | |
| { | |
| result = lift_symbolic_tensor_value(ins.get_literal().get_argument()); | |
| } | |
| if(ins.can_eval()) | |
| { | |
| result = lift_symbolic_tensor_value(ins.eval()); | |
| } |
| register_op_parser(opd.onnx_name, [opd, parser](auto&&... xs) { | ||
| return implicit_multi_op(parser.parse(opd, xs...)); | ||
| }); | ||
| register_op_parser(opd.onnx_name, |
There was a problem hiding this comment.
Is this changes needed now?
|
|
||
| static bool scalar_equal(const scalar& a, const scalar& b) | ||
| { | ||
| return not scalar_less(a, b) and not scalar_less(b, a); |
There was a problem hiding this comment.
is this to avoid float comparison warnings?
3 participants
No description provided.