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.

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

[MAINT] Use std::size_t from cstddef and use dpctl::tensor::ssize_t where ssize_t is used #1950

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ndgrigorian
Copy link
Collaborator

This PR changes the reliance of dpctl headers on size_t being translated by the compiler by instead including cstddef and replacing size_t with std::size_t throughout.

Additionally, using dpctl::tensor::ssize_t is used wherever ssize_t comes up.

  • 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?

Copy link

Copy link

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

@oleksandr-pavlyk
Copy link
Collaborator

@ndgrigorian Looks like these two instances were missed:

(dev_dpctl) opavlyk@opavlyk-mobl:~/repos/dpctl$ grep -r size_t dpctl/tensor/libtensor/ | grep -v "std::size_t" | grep -v "ssize_t" | grep -v "size_to"
dpctl/tensor/libtensor/include/kernels/elementwise_functions/trunc.hpp:                              size_t nelems,
dpctl/tensor/libtensor/include/kernels/elementwise_functions/trunc.hpp:                   size_t nelems,

Copy link

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

@ndgrigorian
Copy link
Collaborator Author

@ndgrigorian Looks like these two instances were missed:

(dev_dpctl) opavlyk@opavlyk-mobl:~/repos/dpctl$ grep -r size_t dpctl/tensor/libtensor/ | grep -v "std::size_t" | grep -v "ssize_t" | grep -v "size_to"
dpctl/tensor/libtensor/include/kernels/elementwise_functions/trunc.hpp:                              size_t nelems,
dpctl/tensor/libtensor/include/kernels/elementwise_functions/trunc.hpp:                   size_t nelems,

I've pushed a change fixing these, good catch!

@oleksandr-pavlyk
Copy link
Collaborator

@ndgrigorian These changes look good to me. I think it is ready for review and ready to be merged in.

One thing to check is whether changes here might conflict with changes in feature/topk branch

@ndgrigorian ndgrigorian marked this pull request as ready for review December 27, 2024 19:26
@ndgrigorian
Copy link
Collaborator Author

@ndgrigorian These changes look good to me. I think it is ready for review and ready to be merged in.

One thing to check is whether changes here might conflict with changes in feature/topk branch

I'll go ahead and check if they do. I will also have to rebase onto master and check for more instances with the accumulator improvement PR merged.

@oleksandr-pavlyk
Copy link
Collaborator

oleksandr-pavlyk commented Dec 27, 2024

The coverage workflow failed with 8 ctest failures related to sycl::kernel_bundle and sycl::kernel

@oleksandr-pavlyk
Copy link
Collaborator

The reason for failures is the std::size_t should not he used in declarations and definitions of EXTERN "C" functions due to name mangling

@oleksandr-pavlyk
Copy link
Collaborator

The C compatible type is defined in "stddef.h"

@ndgrigorian ndgrigorian force-pushed the use-std-size-t-cstddef branch 2 times, most recently from 75f1145 to 976ea23 Compare December 27, 2024 20:53
@ndgrigorian
Copy link
Collaborator Author

The reason for failures is the std::size_t should not he used in declarations and definitions of EXTERN "C" functions due to name mangling

Uses in libsyclinterface have been removed

Copy link

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

Copy link

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

@coveralls
Copy link
Collaborator

coveralls commented Dec 27, 2024

Coverage Status

coverage: 87.659%. remained the same
when pulling ab85a42 on use-std-size-t-cstddef
into f7cb1b1 on master.

@ndgrigorian ndgrigorian force-pushed the use-std-size-t-cstddef branch from 976ea23 to 5afea4d Compare December 28, 2024 00:34
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_367 ran successfully.
Passed: 893
Failed: 3
Skipped: 118

@ndgrigorian ndgrigorian force-pushed the use-std-size-t-cstddef branch from 5afea4d to fe02ce7 Compare December 30, 2024 18:06
@ndgrigorian
Copy link
Collaborator Author

@ndgrigorian These changes look good to me. I think it is ready for review and ready to be merged in.

One thing to check is whether changes here might conflict with changes in feature/topk branch

Unfortunately there are conflicts in merge_sort.hpp and radix_sort.hpp

It may be best to merge this after topk

Copy link

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

@ndgrigorian ndgrigorian force-pushed the use-std-size-t-cstddef branch from fe02ce7 to 7973eb8 Compare December 30, 2024 23:01
Copy link

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

…t is used

Maintains stylistic consistency and removes reliance on compiler interpretation
@ndgrigorian ndgrigorian force-pushed the use-std-size-t-cstddef branch from 7973eb8 to ab85a42 Compare December 31, 2024 02:19
@ndgrigorian
Copy link
Collaborator Author

ndgrigorian commented Dec 31, 2024

@oleksandr-pavlyk
I've also reverted use of std::size_t in _host_task_utils.hpp

In the process, I've also noticed that we are using py::ssize_t as the type for the number of elements in an array for get_size method and in size Python equivalent, while throughout libtensor Python bindings, we use size_t to represent the number of elements.

This inconsistency can be resolved in a later PR, but we should probably use size_t everywhere for this.

Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants