[GPU] Fix dynamic reorder impl selection in shape-of flow#35622
Open
wilson-seok wants to merge 1 commit intoopenvinotoolkit:masterfrom
Open
[GPU] Fix dynamic reorder impl selection in shape-of flow#35622wilson-seok wants to merge 1 commit intoopenvinotoolkit:masterfrom
wilson-seok wants to merge 1 commit intoopenvinotoolkit:masterfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes incorrect GPU reorder implementation selection in shape-of flows and hardens implementation fallback/diagnostics, with added regression tests.
Changes:
- Relaxed the OCL dynamic reorder predicate in shape-of subgraphs to only block when both input/output formats are simple.
- Improved impl selection robustness: continue scanning impl managers when
create()returnsnullptr, and add null-guards in the static compile cache path. - Added regression tests for null-impl fallback and dynamic reorder availability/execution.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/plugins/intel_gpu/tests/unit/test_cases/reorder_gpu_test.cpp | Adds a regression test ensuring dynamic fsv16→bfyx reorder executes correctly in shape-of flow conditions. |
| src/plugins/intel_gpu/tests/unit/module_tests/impls_test.cpp | Adds tests for (a) impl creation returning nullptr fallback and (b) OCL dynamic reorder availability in shape-of subgraphs. |
| src/plugins/intel_gpu/src/graph/registry/reorder_impls.cpp | Adjusts shape-of predicate to avoid incorrectly blocking OCL dynamic reorder when input is non-simple. |
| src/plugins/intel_gpu/src/graph/primitive_inst.cpp | Makes impl selection resilient to create() returning nullptr; adds null guards and richer assertion diagnostics on static impl miss. |
Comment on lines
+390
to
+426
| // Test: verify that find_impl skips impl managers whose create() returns nullptr | ||
| // and continues to the next available impl (fallback behavior) | ||
| TEST(impls_test, impl_create_null_fallback) { | ||
| auto& engine = get_test_engine(); | ||
|
|
||
| // Create a list of impl managers where the first one returns nullptr, | ||
| // and the second one returns a valid impl | ||
| std::vector<std::shared_ptr<ImplementationManager>> test_impls = { | ||
| std::make_shared<NullReturningImplementationManager>(shape_types::static_shape), | ||
| std::make_shared<SomeImplementationManager>(shape_types::static_shape, nullptr), | ||
| }; | ||
|
|
||
| // Simulate find_impl behavior: iterate through impls, skip nullptr results | ||
| kernel_impl_params params; | ||
| params.output_layouts = { layout{{1}, data_types::f32, format::bfyx} }; | ||
| params.input_layouts = { layout{{1}, data_types::f32, format::bfyx} }; | ||
|
|
||
| program p(engine, get_test_default_config(engine)); | ||
| auto prim = std::make_shared<some_primitive>("test_fallback", std::vector<input_info>{}, some_primitive::SomeParameter::SUPPORTED_VALUE_ALL); | ||
| auto& node = p.get_or_create(prim); | ||
| node.recalc_output_layout(); | ||
|
|
||
| std::unique_ptr<primitive_impl> result = nullptr; | ||
| for (auto& impl_manager : test_impls) { | ||
| if ((impl_manager->get_shape_type() & shape_types::static_shape) != shape_types::static_shape) | ||
| continue; | ||
| if (!impl_manager->support_shapes(params)) | ||
| continue; | ||
|
|
||
| auto impl = impl_manager->create(node, params); | ||
| if (impl) { | ||
| result = std::move(impl); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // The first impl returns nullptr, but fallback to the second should succeed |
| std::make_shared<SomeImplementationManager>(shape_types::static_shape, nullptr), | ||
| }; | ||
|
|
||
| // Simulate find_impl behavior: iterate through impls, skip nullptr results |
| node.recalc_output_layout(); | ||
|
|
||
| std::unique_ptr<primitive_impl> result = nullptr; | ||
| for (auto& impl_manager : test_impls) { |
Comment on lines
+392
to
+426
| TEST(impls_test, impl_create_null_fallback) { | ||
| auto& engine = get_test_engine(); | ||
|
|
||
| // Create a list of impl managers where the first one returns nullptr, | ||
| // and the second one returns a valid impl | ||
| std::vector<std::shared_ptr<ImplementationManager>> test_impls = { | ||
| std::make_shared<NullReturningImplementationManager>(shape_types::static_shape), | ||
| std::make_shared<SomeImplementationManager>(shape_types::static_shape, nullptr), | ||
| }; | ||
|
|
||
| // Simulate find_impl behavior: iterate through impls, skip nullptr results | ||
| kernel_impl_params params; | ||
| params.output_layouts = { layout{{1}, data_types::f32, format::bfyx} }; | ||
| params.input_layouts = { layout{{1}, data_types::f32, format::bfyx} }; | ||
|
|
||
| program p(engine, get_test_default_config(engine)); | ||
| auto prim = std::make_shared<some_primitive>("test_fallback", std::vector<input_info>{}, some_primitive::SomeParameter::SUPPORTED_VALUE_ALL); | ||
| auto& node = p.get_or_create(prim); | ||
| node.recalc_output_layout(); | ||
|
|
||
| std::unique_ptr<primitive_impl> result = nullptr; | ||
| for (auto& impl_manager : test_impls) { | ||
| if ((impl_manager->get_shape_type() & shape_types::static_shape) != shape_types::static_shape) | ||
| continue; | ||
| if (!impl_manager->support_shapes(params)) | ||
| continue; | ||
|
|
||
| auto impl = impl_manager->create(node, params); | ||
| if (impl) { | ||
| result = std::move(impl); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // The first impl returns nullptr, but fallback to the second should succeed |
| } | ||
|
|
||
| in_out_fmts_t query_formats(const program_node& node) const override { | ||
| OPENVINO_NOT_IMPLEMENTED; |
Comment on lines
+3195
to
+3197
| OPENVINO_ASSERT(false, "No static impl " + node->id() + ". " + available_impls_info + | ||
| ". Input: " + updated_params.input_layouts[0].to_short_string() + | ||
| ". Output: " + updated_params.output_layouts[0].to_short_string()); |
Comment on lines
+3190
to
+3192
| for (auto& m : m_available_impls) { | ||
| available_impls_info += " {type=" + std::to_string(static_cast<int>(m->get_impl_type())) + | ||
| ", shape=" + std::to_string(static_cast<int>(m->get_shape_type())) + "}"; |
|
|
||
| // Prepare properly formatted fsv16 memory via helper network | ||
| auto input_bfyx = engine.allocate_memory({ov::PartialShape(in_shape), data_types::f32, format::bfyx}); | ||
| std::vector<float> input_data(256); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details:
Tickets:
AI Assistance: