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

thrust::exclusive_scan deduces value type from identity value #836

Open
upsj opened this issue Mar 20, 2023 · 3 comments
Open

thrust::exclusive_scan deduces value type from identity value #836

upsj opened this issue Mar 20, 2023 · 3 comments
Labels
thrust For all items related to Thrust.

Comments

@upsj
Copy link

upsj commented Mar 20, 2023

I ran into a particularly hard-to-spot issue that I would consider a bug in Thrust in the C++ standard 😄 :

#include <thrust/device_vector.h>
#include <thrust/scan.h>
#include <iostream>
#include <limits>
#include <cstdint>

int main() {
        thrust::device_vector<std::int64_t> vec(3);
        vec[0] = std::numeric_limits<std::int64_t>::max() - 1;
        vec[1] = 1;
        vec[2] = 0;
        thrust::exclusive_scan(vec.begin(), vec.end(), vec.begin(), 0);        
        std::cout << vec[0] << '\n';
        std::cout << vec[1] << '\n';
        std::cout << vec[2] << '\n';
}

I would expect this to output 0, LONG_MAX - 1 and LONG_MAX, instead it outputs 0, -2, -1.

EDIT: I checked with std::exclusive_scan, and I get the same behavior, but TBH this seems like a really bug-prone specification to me. I would expect the computational type to be derived based on the return type of the associative operation or the iterator types.

@gevtushenko
Copy link
Collaborator

Hello @upsj!

The standard doesn't specify the accumulator type, so the approach of Thrust current could be better. We've changed the accumulator type in CUB to rely on P2322R6 logic. The plan is to adopt this scheme in Thrust at some point.

@upsj
Copy link
Author

upsj commented Mar 21, 2023

I was pointed to https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0571r2.html#intermediate_unordered, which deals with related issues

@gevtushenko
Copy link
Collaborator

@upsj this paper has not been accepted into the standard yet. It shares the issues of using initial value type as accumulator type with thrust, so we consider P2322R6 instead.

@jarmak-nv jarmak-nv transferred this issue from NVIDIA/thrust Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
thrust For all items related to Thrust.
Projects
Status: Todo
Development

No branches or pull requests

2 participants