-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[GPU] Enable custom op with dynamic shape #30880
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
[GPU] Enable custom op with dynamic shape #30880
Conversation
Signed-off-by: xiping.yan <[email protected]>
Signed-off-by: xiping.yan <[email protected]>
src/plugins/intel_gpu/include/intel_gpu/primitives/custom_gpu_primitive.hpp
Show resolved
Hide resolved
src/plugins/intel_gpu/src/graph/include/custom_gpu_primitive_inst.h
Outdated
Show resolved
Hide resolved
Signed-off-by: xiping.yan <[email protected]>
Signed-off-by: xiping.yan <[email protected]>
src/plugins/intel_gpu/include/intel_gpu/graph/kernel_impl_params.hpp
Outdated
Show resolved
Hide resolved
src/plugins/intel_gpu/include/intel_gpu/primitives/custom_gpu_primitive.hpp
Outdated
Show resolved
Hide resolved
By the way, the current version implements dynamism through kernel recompilation for each new dynamic shape configuration. However, we could support a shape_agnostic kernel version that can be compiled once and reused with any shape. That said, this implementation looks like a good step toward supporting dynamic custom operations |
acd0c40
to
977877a
Compare
Remove overrided get_fake_aligned_params. Signed-off-by: xiping.yan <[email protected]>
hi @sshlyapn , I got your idea, you just want to compile the kernel once for dynamic shape, customer should make sure their kernel is shape_agnostic. 1: Maybe we can merge this solution as first step.(I still need to add test case.) Because we need to update public interface and wiki for the solution 2, so I'd like to merge current solution as first step, and then enable shape_agnostic kernel in the next step. |
Signed-off-by: xiping.yan <[email protected]>
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.
LGTM, thanks.
@peterchen-intel Added test case. Remove draft and ready for review. |
Signed-off-by: xipingyan <[email protected]>
Signed-off-by: xipingyan <[email protected]>
The root cause was unit test did not pass "global work size rule". Signed-off-by: xipingyan <[email protected]>
Enable shape agnostic. Signed-off-by: xipingya <[email protected]>
Hi @sshlyapn , @peterchen-intel |
src/plugins/intel_gpu/tests/functional/custom_op/custom_op_dynamic.cl
Outdated
Show resolved
Hide resolved
@@ -261,6 +295,7 @@ static std::unique_ptr<primitive_impl> create(const custom_gpu_primitive_node& a | |||
namespace detail { | |||
|
|||
attach_custom_gpu_primitive_impl::attach_custom_gpu_primitive_impl() { | |||
implementation_map<custom_gpu_primitive>::add(impl_types::ocl, shape_types::dynamic_shape, create, {}); |
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.
I'd suggest reverting this dynamic impl registration changes, as it's not sufficient to support shape-agnostic kernel logic. It will only make the pipeline think it's requesting dynamic kernel, while it will still get static one - which could lead to issues. So let's merge the initial version first, and then extend it with proper dynamic support
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.
I local test found: it can get dynamic kernel, because I used onetrace to profiling it, after registering the dynamic shape kernel, clBuildProgram
will only be called once, regardless of whether the shape changes or not
If I don't register the dynamic shape kernel, clBuildProgram
is called if the shape changes.
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.
You are right, when I added more dynamic dim to test case, I got some problems.
I just reverted dynamic shape kernel register. Take it as next step.
core.set_property(ov::test::utils::DEVICE_GPU, {{"CONFIG_FILE", TEST_CUSTOM_OP_DYNAMIC_CONFIG_PATH}}); | ||
} | ||
|
||
TEST(CustomOpDynamic, Accuracy) { |
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.
Can you please add smoke_
prefix to the tests name and double check them locally?
[----------] 2 tests from CustomOpDynamic
[ RUN ] CustomOpDynamic.CanReadValidCustomOpConfig
[ OK ] CustomOpDynamic.CanReadValidCustomOpConfig (1 ms)
[ RUN ] CustomOpDynamic.Accuracy
src/plugins/intel_gpu/tests/functional/custom_op/custom_op_dynamic.cpp:119: Failure
Expected equality of these values:
actual[i]
Which is: 0
inp_data[i] * alpha + beta
Which is: 0.40000001
[ FAILED ] CustomOpDynamic.Accuracy (38 ms)
[----------] 2 tests from CustomOpDynamic (39 ms total)
[----------] Global test environment tear-down
[==========] 2 tests from 1 test suite ran. (40 ms total)
[ PASSED ] 1 test.
[ FAILED ] 1 test, listed below:
[ FAILED ] CustomOpDynamic.Accuracy
1 FAILED TEST
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.
Done, strange, I test pass locally,
2: more dynamic dim for test. 3: revert dynamic shape kernel register. 4: Add comment about check if inference output shape. Signed-off-by: xipingya <[email protected]>
Signed-off-by: xipingya <[email protected]>
@xipingyan , can you please check CI test failures?
|
I tested pass at local, ci log show: |
Use "ov::test::utils::createFile() and ov::test::utils::removeFile()" to manage config and cl files. Signed-off-by: xipingya <[email protected]>
…ub.com/xipingyan/openvino into xp/enable_custom_op_with_dynamic_shape
Signed-off-by: xipingya <[email protected]>
Signed-off-by: xipingya <[email protected]>
### Details: - *Wrapper gws and lws calculation, and move it to custom_gpu_primitive.hpp* - *Pass OP(customer provided) to **cldnn::custom_gpu_primitive**, we need to call it(new_op->**validate_and_infer_types()**) to inference real output shape.* - *Add test case.* ### Tickets: - *CVS-170115* --------- Signed-off-by: xiping.yan <[email protected]> Signed-off-by: xipingyan <[email protected]> Signed-off-by: xipingya <[email protected]>
8c41361
Details:
Tickets: