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

[FEA]: Improve binary function objects and replace thrust implementation #1664

Closed
1 task done
miscco opened this issue Apr 24, 2024 · 9 comments · Fixed by #1872
Closed
1 task done

[FEA]: Improve binary function objects and replace thrust implementation #1664

miscco opened this issue Apr 24, 2024 · 9 comments · Fixed by #1872
Assignees
Labels
feature request New feature or request. good first issue Good for newcomers. libcu++ For all items related to libcu++ thrust For all items related to Thrust.

Comments

@miscco
Copy link
Collaborator

miscco commented Apr 24, 2024

Is this a duplicate?

Area

General CCCL

Is your feature request related to a problem? Please describe.

Currently the binary function objects like cuda::std::plus do not universally default their template argument in all standard modes:

#if _CCCL_STD_VER > 2011
template <class _Tp = void>
#else
template <class _Tp>
#endif
struct _LIBCUDACXX_TEMPLATE_VIS plus;

Furthermore those objects are duplicated in thrust.

Describe the solution you'd like

We should replace thrusts versions with an alias declaration and default the cuda::std ones universally

Describe alternatives you've considered

No response

Additional context

No response

@miscco miscco added feature request New feature or request. good first issue Good for newcomers. thrust For all items related to Thrust. libcu++ For all items related to libcu++ labels Apr 24, 2024
@srinivasyadav18
Copy link
Contributor

Hi,

I am interested to work on this issue.

However, I need further context to get started with it.
I have understood the first part, which is defaulting the template argument universally.

Regarding replacing the thrust version with an alias declaration using cuda::std, should I be replacing this code
https://github.com/NVIDIA/cccl/blob/main/thrust/thrust/functional.h#L213-L237

template <typename T = void>
struct plus
{
  typedef T first_argument_type;
  typedef T second_argument_type;
  typedef T result_type;

  _CCCL_EXEC_CHECK_DISABLE
  _CCCL_HOST_DEVICE constexpr T operator()(const T& lhs, const T& rhs) const
  {
    return lhs + rhs;
  }
}; 

to use cuda::std::plus ?

Thanks!

@jrhemstad
Copy link
Collaborator

Hey @srinivasyadav18, thanks for your interest in contributing to CCCL!

You are correct. The basic idea would be to do the following:

Before:

namespace thrust{
template <typename T = void>
struct plus
{
  typedef T first_argument_type;
  typedef T second_argument_type;
  typedef T result_type;

  _CCCL_EXEC_CHECK_DISABLE
  _CCCL_HOST_DEVICE constexpr T operator()(const T& lhs, const T& rhs) const
  {
    return lhs + rhs;
  }
}; 
}

After:

namespace thrust {
   template <typename  T = void>
   using plus = ::cuda::std::plus<T>;
}

@srinivasyadav18
Copy link
Contributor

Hi @jrhemstad,

Thank you providing more context on the issue!

I have replaced most of the function objects in thrust to use cuda::std one's.
But, currently I am stuck with replacing binary_negate function (here), also unary_negate.
As in C++ 20, negators are disabled by default in cuda::std. Hence, its not possible to use it.

Also, the existing thrust::binary_negate does not compile because, the Predicate type does not have any first_argument_type or second_argument_type nested type, becasue these functions objects now use cuda::std:: one's, which are dervided from __binary_function in binary_funcion.h that disable first_argument_type and second_argument_type by default.

Can you please provide more insights on how to proceed with this issue ?
Should we disable the negators in thrust now, like how cuda::std disabled it based on CXX standard ?

Thanks!

@jrhemstad
Copy link
Collaborator

@miscco can you answer @srinivasyadav18's questions?

@miscco
Copy link
Collaborator Author

miscco commented May 27, 2024

@srinivasyadav18 That is indeed unfortunate.

I would for now suggest to just ignore negate and keep them at is. We can move incrementally if we like

@bernhardmgruber
Copy link
Contributor

@srinivasyadav18 how are you getting along? I could use your work, even if it's just partial :)

bernhardmgruber added a commit to bernhardmgruber/cccl that referenced this issue Jun 12, 2024
@bernhardmgruber
Copy link
Contributor

I opened a PR to enable the transparent functors (the = void) also in C++11: #1851

bernhardmgruber added a commit to bernhardmgruber/cccl that referenced this issue Jun 12, 2024
@srinivasyadav18
Copy link
Contributor

@bernhardmgruber Apologies for late response. I have replace most of function objects to use the ::cuda::std one's. But there are still lot more places where I need to remove the use of first_argument_type, second_arguement_type and result_type nested types from the binary functions.

bernhardmgruber added a commit to bernhardmgruber/cccl that referenced this issue Jun 12, 2024
@bernhardmgruber
Copy link
Contributor

Awesome! So your changes should not interfere with any of mine. Make sure to rebase on the latest tip of main when #1851 lands!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request. good first issue Good for newcomers. libcu++ For all items related to libcu++ thrust For all items related to Thrust.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants