Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix gather kernel bug for handling 5 and 6 dimensions #26894

Merged
merged 4 commits into from
Oct 14, 2024
Merged

Fix gather kernel bug for handling 5 and 6 dimensions #26894

merged 4 commits into from
Oct 14, 2024

Conversation

clee30
Copy link
Contributor

@clee30 clee30 commented Oct 3, 2024

Details:

The calculation to generate INDICES_INDEX_ORDER is wrong.

For 5 dimension, INDICES_INDEX_ORDER should be
b, f, 0, z, 0 or b, f, 0, 0, z

For 6 dimension, INDICES_INDEX_ORDER should be
b, f, 0, w, z, 0 or b, f, 0, w, 0,z

To simplify the generation, generate b,f,0,z,0 for 5 dims and b,f,0,w,z,0 for 6 dimsm

Tickets:

@clee30 clee30 requested review from a team as code owners October 3, 2024 08:14
@github-actions github-actions bot added the category: GPU OpenVINO GPU plugin label Oct 3, 2024
@sys-openvino-ci sys-openvino-ci added the ExternalIntelPR External contributor from Intel label Oct 3, 2024
@vladimir-paramuzov
Copy link
Contributor

build_jenkins

@clee30 clee30 requested a review from a team as a code owner October 3, 2024 10:03
@github-actions github-actions bot added the category: TF FE OpenVINO TensorFlow FrontEnd label Oct 3, 2024
@vladimir-paramuzov
Copy link
Contributor

build_jenkins

rkazants
rkazants approved these changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@rkazants rkazants left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From TF perspective, LGTM

@vladimir-paramuzov
Copy link
Contributor

@clee30 Looks like some unit tests fail:

[----------] 1 test from gather8_gpu_fp16
[ RUN      ] gather8_gpu_fp16.d323_axisY_bdim_m1
src/plugins/intel_gpu/tests/unit/test_cases/gather_gpu_test.cpp:454: Failure
Expected equality of these values:
  expected_results[i]
    Which is: 27
  half_to_float(output_ptr[i])
    Which is: 31
[  FAILED  ] gather8_gpu_fp16.d323_axisY_bdim_m1 (457 ms)
[----------] 1 test from gather8_gpu_fp16 (457 ms total)

[----------] 6 tests from gather7_gpu_fp16
[ RUN      ] gather7_gpu_fp16.d222_axisX_bdim_m1
** Segmentation fault
...

@clee30
Copy link
Contributor Author

clee30 commented Oct 4, 2024

@clee30 Looks like some unit tests fail:

[----------] 1 test from gather8_gpu_fp16
[ RUN      ] gather8_gpu_fp16.d323_axisY_bdim_m1
src/plugins/intel_gpu/tests/unit/test_cases/gather_gpu_test.cpp:454: Failure
Expected equality of these values:
  expected_results[i]
    Which is: 27
  half_to_float(output_ptr[i])
    Which is: 31
[  FAILED  ] gather8_gpu_fp16.d323_axisY_bdim_m1 (457 ms)
[----------] 1 test from gather8_gpu_fp16 (457 ms total)

[----------] 6 tests from gather7_gpu_fp16
[ RUN      ] gather7_gpu_fp16.d222_axisX_bdim_m1
** Segmentation fault
...

Thanks. The gather unit tests are passing. Will push the fix in soon.

@clee30
Fix gather kernel bug for handling case for 5 and 6 input dimensions
ed781cf 
The earlier calculation to generate INDICES_INDEX_ORDER is wrong for 5/6
dimension.

For 5 dimension case, INDICES_INDEX_ORDER should be
b, f, 0, z, 0 or b, f, 0, 0, z

For 6 dimension case, INDICES_INDEX_ORDER should be
b, f, 0, w, z, 0 or b, f, 0, w, 0,z

We don't have much test case to handle various type of 5 or 6 dimension,
will use a hard coded INDICES_INDEX_ORDER to fix the AdjustHue test.
@clee30
Copy link
Contributor Author

clee30 commented Oct 5, 2024

@clee30 Looks like some unit tests fail:

[----------] 1 test from gather8_gpu_fp16
[ RUN      ] gather8_gpu_fp16.d323_axisY_bdim_m1
src/plugins/intel_gpu/tests/unit/test_cases/gather_gpu_test.cpp:454: Failure
Expected equality of these values:
  expected_results[i]
    Which is: 27
  half_to_float(output_ptr[i])
    Which is: 31
[  FAILED  ] gather8_gpu_fp16.d323_axisY_bdim_m1 (457 ms)
[----------] 1 test from gather8_gpu_fp16 (457 ms total)

[----------] 6 tests from gather7_gpu_fp16
[ RUN      ] gather7_gpu_fp16.d222_axisX_bdim_m1
** Segmentation fault
...

Fixed.

@vladimir-paramuzov
Copy link
Contributor

build_jenkins

@vladimir-paramuzov vladimir-paramuzov added this pull request to the merge queue Oct 14, 2024
Merged via the queue into openvinotoolkit:master with commit 9c432a3 Oct 14, 2024
143 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladimir-paramuzov vladimir-paramuzov vladimir-paramuzov left review comments

@rkazants rkazants rkazants approved these changes

Assignees
No one assigned
Labels
category: GPU OpenVINO GPU plugin category: TF FE OpenVINO TensorFlow FrontEnd ExternalIntelPR External contributor from Intel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@clee30 @vladimir-paramuzov @rkazants @sys-openvino-ci @p-durandin