[CPU][DEBUG CAPS] Fix failure on models with dynamic type constant nodes#34000
[CPU][DEBUG CAPS] Fix failure on models with dynamic type constant nodes#34000aobolensk merged 5 commits intoopenvinotoolkit:masterfrom
Conversation
32ee451 to
795459e
Compare
src/core/src/op/constant.cpp
Outdated
|
|
||
| template <ov::element::Type_t ET, typename std::enable_if<ET == element::dynamic>::type* = nullptr> | ||
| static result_type visit(const void* const ptr, const size_t index) { | ||
| return ""; |
There was a problem hiding this comment.
what about result_type{} or {}
src/core/src/op/constant.cpp
Outdated
| } | ||
|
|
||
| template <ov::element::Type_t ET, typename std::enable_if<ET == element::dynamic>::type* = nullptr> | ||
| static result_type visit(const void* const ptr, const size_t num_elements, std::vector<std::string>& strs) {} |
There was a problem hiding this comment.
the variable names can be removed or [[maybe_unused]]
There was a problem hiding this comment.
Removed variable names
There was a problem hiding this comment.
Pull request overview
Fixes ValuesToString / ValueToString failures when encountering ov::element::dynamic constants by adding dynamic-type handling to the visitor dispatch.
Changes:
- Adds
element::dynamicoverloads forValueToStringandValuesToString. - Expands
IfTypeOfdispatch to includedynamicso dynamic constants don’t route to unsupported behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/core/src/op/constant.cpp
Outdated
| static result_type visit(const void* const ptr, const size_t index) { | ||
| return element::iterator<ET>(ptr)[index]; | ||
| } | ||
|
|
||
| template <ov::element::Type_t ET, typename std::enable_if<ET == element::dynamic>::type* = nullptr> | ||
| static result_type visit(const void* const ptr, const size_t index) { | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
For ET == element::dynamic, both visit templates are viable (the unconstrained template also matches), which can lead to an ambiguous overload or still instantiate element::iterator<ET> for dynamic. Constrain the non-dynamic overload (e.g., ET != element::dynamic) or merge into a single template using if constexpr to avoid compiling the iterator path for dynamic.
src/core/src/op/constant.cpp
Outdated
| } | ||
|
|
||
| template <ov::element::Type_t ET, typename std::enable_if<ET == element::dynamic>::type* = nullptr> | ||
| static result_type visit(const void* const ptr, const size_t num_elements, std::vector<std::string>& strs) {} |
There was a problem hiding this comment.
The dynamic overload leaves strs unchanged, so get_value_strings() can return an empty vector even when shape_size(m_shape) is non-zero. This creates inconsistent behavior vs. other element types (which always append num_elements entries). Consider appending/resizing strs with num_elements placeholder strings (e.g., empty strings) so the output cardinality remains consistent.
| static result_type visit(const void* const ptr, const size_t num_elements, std::vector<std::string>& strs) {} | |
| static result_type visit(const void* const /*ptr*/, | |
| const size_t num_elements, | |
| std::vector<std::string>& strs) { | |
| // For dynamic element type, we cannot obtain concrete values, but we still | |
| // need to keep the output cardinality consistent with num_elements. | |
| // Append num_elements placeholder (empty) strings. | |
| strs.insert(strs.end(), num_elements, std::string{}); | |
| } |
src/core/src/op/constant.cpp
Outdated
| } | ||
|
|
||
| template <ov::element::Type_t ET, typename std::enable_if<ET == element::dynamic>::type* = nullptr> | ||
| static result_type visit(const void* const ptr, const size_t num_elements, std::vector<std::string>& strs) {} |
There was a problem hiding this comment.
This empty-body overload will typically trigger unused-parameter warnings for ptr, num_elements, and strs. To keep builds warning-clean, either omit parameter names in the signature or explicitly mark them unused (e.g., (void)ptr;).
| static result_type visit(const void* const ptr, const size_t num_elements, std::vector<std::string>& strs) {} | |
| static result_type visit(const void* const ptr, const size_t num_elements, std::vector<std::string>& strs) { | |
| (void)ptr; | |
| (void)num_elements; | |
| (void)strs; | |
| } |
There was a problem hiding this comment.
Removed variable names instead
ca3a6b1 to
d1f1d70
Compare
src/core/src/op/constant.cpp
Outdated
| return element::iterator<ET>(ptr)[index]; | ||
| } | ||
|
|
||
| template <ov::element::Type_t ET, typename std::enable_if<ET == element::dynamic>::type* = nullptr> |
There was a problem hiding this comment.
Can you add test?
The constant cannot be created with dynamic type the could should be not required at all
There was a problem hiding this comment.
This is IR after CPU specific transformations
There was a problem hiding this comment.
The dynamic type mean no data so still member which access data should throw as incorrect use.
The public IR shouldn't have such constant it looks like it has to be handled by CPU.
There was a problem hiding this comment.
Reverted core changes and re-apply previous CPU-only solution
2c2a56b to
58819b2
Compare
src/core/src/op/constant.cpp
Outdated
| std::vector<std::string> out; | ||
| using namespace ov::element; | ||
| IfTypeOf<SUPPORTED_ET, string>::apply<ValuesToString>(get_element_type(), get_data_ptr(), shape_size(m_shape), out); | ||
| IfTypeOf<SUPPORTED_ET, dynamic, string>::apply<ValuesToString>(get_element_type(), |
There was a problem hiding this comment.
The dynamic has to be remove, there still should be an exception
There was a problem hiding this comment.
@EgorDuplensky shall we switch to the previous CPU-only related fix?
src/core/src/op/constant.cpp
Outdated
| return element::iterator<ET>(ptr)[index]; | ||
| } | ||
|
|
||
| template <ov::element::Type_t ET, typename std::enable_if<ET == element::dynamic>::type* = nullptr> |
There was a problem hiding this comment.
The dynamic type mean no data so still member which access data should throw as incorrect use.
The public IR shouldn't have such constant it looks like it has to be handled by CPU.
fe54820 to
9e7a4f2
Compare
| for (const auto& v : constop->get_value_strings()) { | ||
| os << sep << v; | ||
| sep = ","; | ||
| } |
There was a problem hiding this comment.
| for (const auto& v : constop->get_value_strings()) { | |
| os << sep << v; | |
| sep = ","; | |
| } | |
| os << ov::util::join(constop->get_value_strings(), ","); |
Optional, or use default separator ", "?
### Details: Filling the testing gap for `dynamic` type constants observed in #34000 ### Tickets: - N/A
### Details: Filling the testing gap for `dynamic` type constants observed in openvinotoolkit#34000 ### Tickets: - N/A
b817324
### Details: Filling the testing gap for `dynamic` type constants observed in openvinotoolkit#34000 ### Tickets: - N/A
…des (openvinotoolkit#34000) ### Tickets: - N/A

Tickets: