Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Fix select if for mixed types #444

Merged

Conversation

gevtushenko
Copy link
Collaborator

During three-way partitioning development I've noticed that there's a bug in the select if implementation. It used output type to load data from input iterator. Recently, @elstehle brought the very same issue. Here's a code sample that fails to compile. This fact limits the applicability of the partitioning/selection facilities:

#include <iostream>

#include <thrust/count.h>
#include <thrust/iterator/zip_iterator.h>
#include <thrust/iterator/transform_output_iterator.h>
#include <thrust/iterator/discard_iterator.h>
#include <thrust/device_vector.h>

#include <cub/device/device_select.cuh>

struct pair_to_col {
    __host__ __device__ int operator()(const thrust::tuple<int, double> &in) {
        return thrust::get<0>(in);
    }
};

struct select {
    __host__ __device__ bool operator()(const thrust::tuple<int, double> &in) {
        return static_cast<double>(thrust::get<0>(in)) > thrust::get<1>(in);
    }
};

int main() {
    const std::size_t n = 1024;
    thrust::device_vector<int> col_a(n, 42);
    thrust::device_vector<double> col_b(n, 4.2);

    thrust::device_vector<int> result(n, 42);

    auto in = thrust::make_zip_iterator(col_a.begin(), col_b.begin());
    auto out = thrust::make_transform_output_iterator(result.begin(), pair_to_col{});

    std::size_t tmp_storage_size{};
    cub::DeviceSelect::If(
            nullptr, tmp_storage_size, in, out, thrust::make_discard_iterator(), n, select{});

    thrust::device_vector<char> tmp_storage(tmp_storage_size);
    cub::DeviceSelect::If(
            thrust::raw_pointer_cast(tmp_storage.data()), tmp_storage_size, in, out, thrust::make_discard_iterator(), n, select{});

    if (thrust::count(result.begin(), result.end(), 42) == n) {
        std::cout << "OK" << std::endl;
    } else {
        std::cout << "FAIL" << std::endl;
    }

    return 0;
}

This PR addresses this issue, allowing different value types for input and output iterators. It's worth mentioning that the implementation used to have correct types up to the following commit which doesn't contain any mentions of the value types in its description. So my guess whould be that it was a typo rather than an intent.

I've also noticed that the radix sort test fails to compile on windows. I've fixed a few conversion issues here.

@gevtushenko gevtushenko added the type: bug: functional Does not work as intended. label Mar 16, 2022
@alliepiper alliepiper added this to the 1.17.0 milestone Mar 16, 2022
@alliepiper
Copy link
Collaborator

@senior-zero Sounds like this may also fix #406, can you confirm?

Copy link
Collaborator

@elstehle elstehle left a comment

Choose a reason for hiding this comment

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

Thanks for jumping onto this so quickly, @senior-zero! LGTM. This will definitely help my use case. Given that the change also follows the behaviour we have in the prefix scan, I think it makes sense too. I'm curious if, originally, making InputIt::value_type the internal_type was a conscious decision and if there's rationale behind.

Comment on lines +132 to +133
// The input value type
using InputT = cub::detail::value_t<InputIteratorT>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch 👍 I probably would have missed adjusting the tuning policies.

@gevtushenko
Copy link
Collaborator Author

@senior-zero Sounds like this may also fix #406, can you confirm?

Yes, I confirm. After the fix, the code from #406 gets compiled.

cub/device/dispatch/dispatch_radix_sort.cuh Outdated Show resolved Hide resolved
cub/device/dispatch/dispatch_radix_sort.cuh Outdated Show resolved Hide resolved
test/test_device_radix_sort.cu Show resolved Hide resolved
test/test_device_radix_sort.cu Show resolved Hide resolved
test/test_device_radix_sort.cu Show resolved Hide resolved
test/test_device_radix_sort.cu Show resolved Hide resolved
test/test_device_select_if.cu Outdated Show resolved Hide resolved
test/test_device_select_if.cu Show resolved Hide resolved
@alliepiper alliepiper linked an issue Mar 16, 2022 that may be closed by this pull request
@gevtushenko gevtushenko force-pushed the fix-main/github/select_if_mixed_types branch from f52cfbb to 94c3e1f Compare March 17, 2022 12:19
Copy link
Collaborator

@alliepiper alliepiper left a comment

Choose a reason for hiding this comment

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

LGTM -- I'll start tests in a moment.

alliepiper added a commit to alliepiper/thrust that referenced this pull request Mar 17, 2022
@alliepiper
Copy link
Collaborator

gpuCI: NVIDIA/thrust#1633
DVS CL: 31093334

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug: functional Does not work as intended.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cub::DeviceSelect::Unique doesn't work with thrust::discard_iterator
3 participants