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

Work-around for issue with CPU driver, add tests #1951

Merged
merged 8 commits into from
Jan 4, 2025
Merged

Work-around for issue with CPU driver, add tests #1951

merged 8 commits into from
Jan 4, 2025
+300 −93

Conversation

oleksandr-pavlyk
Copy link
Collaborator

@oleksandr-pavlyk oleksandr-pavlyk commented Dec 29, 2024 •
edited
Loading

Due to a reported issue with OpenCL:CPU device implementation, the kernel for short inputs and short signed integral types may produce incorrect results when compiled for short SIMD width (as chosen automatically for AMD EPYC 7763 64 Processors CPU).

A work-around is to introduce a redundant barrier call (only for CPU devices when sub-group is short).

Also edits made while triaging the issue are applied: counters for short input arrays are stored in 16-bit unsigned integers, rather than in 32-bit ones, uint16_t replaced with std::uint16_t, etc.

Tests for tensor.top_k were expanded to ensure 100% coverage.

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to an issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • Have you checked performance impact of proposed changes?
  • Have you added documentation for your changes, if necessary?
  • Have you added your changes to the changelog?
  • If this PR is a work in progress, are you opening the PR as a draft?

@oleksandr-pavlyk oleksandr-pavlyk changed the title Introduce local skip function to work-around issue with CPU driver Introduce local skip function to work-around issue with CPU driver, add tests Dec 29, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 29, 2024 •
edited
Loading

Deleted rendered PR docs from intelpython.github.com/dpctl, latest should be updated shortly. 🤞

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_388 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_389 ran successfully.
Passed: 892
Failed: 4
Skipped: 118

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_390 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@oleksandr-pavlyk
Copy link
Collaborator Author

@ndgrigorian For some reason pytest fails to recognize the added command-line option, and I can reproduce that locally (install the build artifact created by CI and try the command CI is trying from an empty folder, but I do not understand why it fails).

@ndgrigorian
Copy link
Collaborator

@ndgrigorian For some reason pytest fails to recognize the added command-line option, and I can reproduce that locally (install the build artifact created by CI and try the command CI is trying from an empty folder, but I do not understand why it fails).

Yes, and it works fine in the coverage CI as well.

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_391 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_391 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_392 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_394 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_393 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_393 ran successfully.
Passed: 895
Failed: 1
Skipped: 118

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_393 ran successfully.
Passed: 895
Failed: 1
Skipped: 118

@oleksandr-pavlyk
Copy link
Collaborator Author

@ndgrigorian The CI is green now

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_394 ran successfully.
Passed: 895
Failed: 1
Skipped: 118

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_396 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 2, 2025

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_399 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 3, 2025

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_400 ran successfully.
Passed: 895
Failed: 1
Skipped: 118

@oleksandr-pavlyk
Copy link
Collaborator Author

oleksandr-pavlyk commented Jan 3, 2025 •
edited
Loading

All lines of top_k function implementation are now covered.

I think this PR is ready to be merged into the targeted base branch.

Let me only rebase to squash some commits. (EDIT: Done)

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 3, 2025

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_396 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 4, 2025

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_398 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@oleksandr-pavlyk oleksandr-pavlyk changed the title Introduce local skip function to work-around issue with CPU driver, add tests Work-around for issue with CPU driver, add tests Jan 4, 2025
@oleksandr-pavlyk
Use Ubuntu 24.04 to run the OS-LLVM workflow to build dpctl with nigh…
7436de6 
…tly SYCL bundle DPC++ compiler
@oleksandr-pavlyk
No need to install libtinfo5 on Ubuntu 24.04
bff71fc
@oleksandr-pavlyk
Fixed blunder in work-item id to data_id computation
addb341 
gid-lane_id is already a multiple of sg_size.
@oleksandr-pavlyk
Replace map_back_impl in sort_utils
399cdd1 
Change kernel to process few data elements in the work-item.
@oleksandr-pavlyk
Use get_global_linear_id instead of get_global_id and rely on implici…
1a64014 
…t conversion
@oleksandr-pavlyk
Counters in one-workgroup kernel to use uint16_t from uint32_t
3bd3338 
Counters can not exceed uint16_t max, because the kernel
assumes that the number of elements to sort fits into uint16_t.
The change reduces the kernel SLM footprint.

Also, remove use of std::move, uint16_t->std::uint16_t, etc

Replace size_t->std::size_t, uint32_t->std::uint32_t

Use `if constexpr` in order-preservign-cast for better readability.
@oleksandr-pavlyk
Apply work-around for failing tests with CPU device and short sub-groups
7c7d8f9 
The team developing OpenCL:CPU device runtime and compiler was notified.
See CMPLRLLVM-64592

Once fixed, the work-around should be removed.
@oleksandr-pavlyk
Remove skipping of tests for i1/i2 dtypes since work-around
e1b7540 
was applied in C++.

Add tests for 2d input arrays, for axis=0 and axis=1

Add a test for non-contiguous input, 0d input, validation

100% coverage of top_k function implementation achieved
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 4, 2025

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_395 ran successfully.
Passed: 895
Failed: 1
Skipped: 118

ndgrigorian
ndgrigorian approved these changes Jan 4, 2025
View reviewed changes
Copy link
Collaborator

@ndgrigorian ndgrigorian left a comment

Choose a reason for hiding this comment

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

Tests now passing, LGTM!

@oleksandr-pavlyk oleksandr-pavlyk merged commit 2865f27 into feature/topk Jan 4, 2025
58 of 61 checks passed
@oleksandr-pavlyk oleksandr-pavlyk deleted the improve-top_k-coverage branch January 4, 2025 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndgrigorian ndgrigorian ndgrigorian approved these changes

Assignees
No one assigned
Labels
tensor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@oleksandr-pavlyk @ndgrigorian