-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[core] Propagate value bounds through ConvertLike #36168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |||||
| #include "common_test_utils/test_assertions.hpp" | ||||||
| #include "common_test_utils/type_prop.hpp" | ||||||
| #include "openvino/op/concat.hpp" | ||||||
| #include "openvino/op/convert_like.hpp" | ||||||
| #include "openvino/op/subtract.hpp" | ||||||
| #include "openvino/op/util/framework_node.hpp" | ||||||
|
|
||||||
|
|
@@ -86,3 +87,29 @@ TEST(BoundEvaluatorTest, no_exception_on_single_bound) { | |||||
| OV_ASSERT_NO_THROW(sub->evaluate_upper(output)); | ||||||
| EXPECT_EQ(o_[0], 10); | ||||||
| } | ||||||
|
|
||||||
| // ConvertLike must propagate value bounds (like its v0::Convert sibling), so that a | ||||||
| // ConvertLike(ShapeOf(static), like) node yields a concrete bound during value propagation. Without it, | ||||||
| // bounds-based shape inference (e.g. v8::Slice `stop`) leaves downstream shapes dynamic even when the | ||||||
| // value is statically determinable. | ||||||
| TEST(BoundEvaluatorTest, convert_like_propagates_bounds_from_shape_of) { | ||||||
| const auto data = std::make_shared<Parameter>(element::f32, PartialShape{1, 128, 4, 128}); | ||||||
| const auto shape_of = std::make_shared<ShapeOf>(data); // i64: [1, 128, 4, 128] | ||||||
| const auto like = Constant::create(element::i32, Shape{4}, {0, 0, 0, 0}); | ||||||
| const auto convert_like = std::make_shared<op::v1::ConvertLike>(shape_of, like); | ||||||
|
|
||||||
| const auto bounds = ov::util::evaluate_both_bounds(convert_like); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| ASSERT_TRUE(static_cast<bool>(bounds.first)); | ||||||
| ASSERT_TRUE(static_cast<bool>(bounds.second)); | ||||||
| EXPECT_EQ(bounds.first.get_element_type(), element::i32); | ||||||
| EXPECT_EQ(bounds.second.get_element_type(), element::i32); | ||||||
|
|
||||||
| const std::vector<int32_t> expected{1, 128, 4, 128}; | ||||||
| ASSERT_EQ(bounds.first.get_shape(), ov::Shape{expected.size()}); | ||||||
| ASSERT_EQ(bounds.second.get_shape(), ov::Shape{expected.size()}); | ||||||
|
|
||||||
| const auto* lower = bounds.first.data<int32_t>(); | ||||||
| const auto* upper = bounds.second.data<int32_t>(); | ||||||
| EXPECT_EQ(std::vector<int32_t>(lower, lower + expected.size()), expected); | ||||||
| EXPECT_EQ(std::vector<int32_t>(upper, upper + expected.size()), expected); | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,14 @@ | |
|
|
||
| #include "common_test_utils/test_assertions.hpp" | ||
| #include "common_test_utils/type_prop.hpp" | ||
| #include "openvino/op/add.hpp" | ||
| #include "openvino/op/broadcast.hpp" | ||
| #include "openvino/op/convert.hpp" | ||
| #include "openvino/op/convert_like.hpp" | ||
| #include "openvino/op/less.hpp" | ||
| #include "openvino/op/minimum.hpp" | ||
| #include "openvino/op/select.hpp" | ||
| #include "openvino/op/shape_of.hpp" | ||
| #include "openvino/op/subtract.hpp" | ||
| #include "sequence_generator.hpp" | ||
|
|
||
|
|
@@ -1180,6 +1187,79 @@ TEST(type_prop, slice_v8_start_stop_is_shape_of_with_bounds) { | |
| EXPECT_THAT(get_shape_symbols(slice->get_output_partial_shape(0)), Each(nullptr)); | ||
| } | ||
|
|
||
| // Reproduces the TF/TFLite Slice `stop` cascade with statically determinable inputs: | ||
| // stop = Select(Less(size, 0), ConvertLike(ShapeOf(data), size), Add(start, size)) | ||
| // `start`/`size`/`step` are static Constants and `data` is a Parameter with a static shape, so the whole | ||
| // `stop` subgraph is statically determinable via bounds propagation. Slice shape inference resolves | ||
| // `stop` via bounds propagation (evaluate_lower/upper), not ConstantFolding, so the output must come out | ||
| // static [1,128,4,128] without any folding pass. This exercises ConvertLike value bound evaluation: if | ||
| // ConvertLike does not propagate bounds, the propagation breaks before Select and the Slice output is | ||
| // left dynamic. | ||
| TEST(type_prop, slice_v8_stop_select_cascade_static_inputs_is_static) { | ||
| constexpr auto et = element::i32; | ||
| const auto data = std::make_shared<op::v0::Parameter>(element::f32, PartialShape{1, 128, 8, 256}); | ||
| const auto start = op::v0::Constant::create(et, Shape{4}, {0, 0, 0, 0}); | ||
| const auto size = op::v0::Constant::create(et, Shape{4}, {1, 128, 4, 128}); | ||
| const auto step = op::v0::Constant::create(et, Shape{4}, {1, 1, 1, 1}); | ||
|
|
||
| const auto zero = op::v0::Constant::create(et, Shape{}, {0}); | ||
| const auto negative_size_mask = std::make_shared<op::v1::Less>(size, zero); | ||
| const auto shape_of = std::make_shared<op::v3::ShapeOf>(data); | ||
| const auto stop_neg = std::make_shared<op::v1::ConvertLike>(shape_of, size); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you try having Convert here and have no changes done in convert_like.hpp to confirm this is ConvertLike's problem?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes — confirmed both directions. I swapped ConvertLike for v0::Convert in this exact test with the convert_like.cpp/.hpp change reverted, and the Slice output already comes out static [1,128,4,128] — because Convert already implements evaluate_lower/evaluate_upper/evaluate_symbol (convert.cpp:274-289). With ConvertLike and the fix reverted, the same Slice stays dynamic; with the fix it's static again. So the only missing piece was ConvertLike's value-bound evaluation, which is exactly what this PR adds by delegating to v0::Convert. To lock this in I added a Convert-based control test (slice_v8_stop_select_cascade_convert_is_static) next to the existing Minimum(ShapeOf) control — it's the same cascade but with the then-branch built from v0::Convert, and it stays static regardless of ConvertLike's own bound support. |
||
| const auto stop_pos = std::make_shared<op::v1::Add>(start, size); | ||
| const auto stop = std::make_shared<op::v1::Select>(negative_size_mask, stop_neg, stop_pos); | ||
|
|
||
| const auto slice = std::make_shared<op::v8::Slice>(data, start, stop, step); | ||
|
|
||
| EXPECT_TRUE(slice->get_output_partial_shape(0).is_static()); | ||
| EXPECT_EQ(slice->get_output_partial_shape(0), PartialShape({1, 128, 4, 128})); | ||
| } | ||
|
|
||
| // Control for the cascade test above: a Slice whose `stop` is Minimum(ShapeOf(data), const) already | ||
| // resolves to a static output, because Minimum (BinaryElementwiseArithmetic) supports bound evaluation. | ||
| // This pins the dynamism to ops that lack bound evaluation (the Select+ConvertLike cascade), not to | ||
| // ShapeOf usage in general. | ||
| TEST(type_prop, slice_v8_stop_minimum_shapeof_is_static) { | ||
| constexpr auto et = element::i64; | ||
| const auto data = std::make_shared<op::v0::Parameter>(element::f32, PartialShape{1, 128, 8, 256}); | ||
| const auto start = op::v0::Constant::create(et, Shape{4}, {0, 0, 0, 0}); | ||
| const auto step = op::v0::Constant::create(et, Shape{4}, {1, 1, 1, 1}); | ||
|
|
||
| const auto shape_of = std::make_shared<op::v3::ShapeOf>(data); | ||
| const auto cap = op::v0::Constant::create(et, Shape{4}, {1, 128, 4, 128}); | ||
| const auto stop = std::make_shared<op::v1::Minimum>(shape_of, cap); | ||
|
|
||
| const auto slice = std::make_shared<op::v8::Slice>(data, start, stop, step); | ||
|
|
||
| EXPECT_TRUE(slice->get_output_partial_shape(0).is_static()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This redundant as lower the output shape is compared with static shape |
||
| EXPECT_EQ(slice->get_output_partial_shape(0), PartialShape({1, 128, 4, 128})); | ||
| } | ||
|
|
||
| // Control pinning the cascade dynamism to ConvertLike specifically: the same cascade as | ||
| // slice_v8_stop_select_cascade_static_inputs_is_static, but with the `size < 0` branch built from | ||
| // v0::Convert instead of v1::ConvertLike. v0::Convert has always implemented value bound evaluation | ||
| // (evaluate_lower/upper/symbol), so this Slice resolves to a static output regardless of ConvertLike's | ||
| // own bound-evaluation support. It is the behavioral reference that ConvertLike now mirrors. | ||
| TEST(type_prop, slice_v8_stop_select_cascade_convert_is_static) { | ||
| constexpr auto et = element::i32; | ||
| const auto data = std::make_shared<op::v0::Parameter>(element::f32, PartialShape{1, 128, 8, 256}); | ||
| const auto start = op::v0::Constant::create(et, Shape{4}, {0, 0, 0, 0}); | ||
| const auto size = op::v0::Constant::create(et, Shape{4}, {1, 128, 4, 128}); | ||
| const auto step = op::v0::Constant::create(et, Shape{4}, {1, 1, 1, 1}); | ||
|
|
||
| const auto zero = op::v0::Constant::create(et, Shape{}, {0}); | ||
| const auto negative_size_mask = std::make_shared<op::v1::Less>(size, zero); | ||
| const auto shape_of = std::make_shared<op::v3::ShapeOf>(data); | ||
| const auto stop_neg = std::make_shared<op::v0::Convert>(shape_of, et); | ||
| const auto stop_pos = std::make_shared<op::v1::Add>(start, size); | ||
| const auto stop = std::make_shared<op::v1::Select>(negative_size_mask, stop_neg, stop_pos); | ||
|
|
||
| const auto slice = std::make_shared<op::v8::Slice>(data, start, stop, step); | ||
|
|
||
| EXPECT_TRUE(slice->get_output_partial_shape(0).is_static()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check is not required. |
||
| EXPECT_EQ(slice->get_output_partial_shape(0), PartialShape({1, 128, 4, 128})); | ||
| } | ||
|
|
||
| TEST(type_prop, slice_v8_unknowns_axes) { | ||
| const auto data = std::make_shared<ov::op::v0::Parameter>(element::i64, Shape{5, 10, 15}); | ||
| const auto start = std::make_shared<ov::op::v0::Parameter>(element::i64, PartialShape{-1}); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work for this function, there should be no need to create full validated operator to use bound evalute.