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

Sequence: Specialise compute_sequence_values for builtin arithmetic types #1507

Merged
merged 3 commits into from
Oct 6, 2021

Conversation

bjude
Copy link
Contributor

@bjude bjude commented Aug 22, 2021

Only explicitly cast to T when we're dealing with builtin arithmetic types and just perform the init + step * i operation for all other types. This enables thrust::sequence to work for types without a conversion from std::size_t.

Addresses issue #1498

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

@alliepiper alliepiper self-assigned this Sep 21, 2021
@alliepiper alliepiper added testing: gpuCI in progress Started gpuCI testing. testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). labels Sep 21, 2021
@alliepiper alliepiper added this to the 1.15.0 milestone Sep 21, 2021
@alliepiper alliepiper linked an issue Sep 21, 2021 that may be closed by this pull request
@alliepiper
Copy link
Collaborator

Thanks for the patch, and sorry about the review delay!

LGTM -- I just pushed a small fixup commit to add a missing newline. Starting tests now.

DVS CL: 30441766

run tests

@alliepiper
Copy link
Collaborator

There are some build failures to address before this is merged:

../testing/sequence.cu: In function 'void TestSequenceNoSizeTConversion()':
../testing/sequence.cu:152:20: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
     for (auto i = 0; i < m.size(); ++i)
                      ~~^~~~~~~~~~

We also need to support C++11, so the enable_if_t will need to be changed to enable_if.

../thrust/system/detail/generic/sequence.inl:69:34: error: ‘enable_if_t’ is not a member of ‘std’
 struct compute_sequence_value<T, std::enable_if_t<std::is_arithmetic<T>::value>>
                                  ^~~

@bjude bjude force-pushed the sequence_no_size_t_conversion branch 2 times, most recently from 7cf7855 to 8743aec Compare September 22, 2021 00:15
bjude and others added 2 commits September 22, 2021 10:16
…ypes

Only explicitly cast to T when we're dealing with builtin arithmetic types and just perform the `init + step * i` operation for all other types. This enables `thrust::sequence` to work for types without a conversion from `std::size_t`.

Addresses issue NVIDIA#1498
(cherry picked from commit c4f4d59)
@alliepiper
Copy link
Collaborator

Thanks for the quick fix :) Restarting tests.

DVS CL: 30452736

run tests

testing/sequence.cu Outdated Show resolved Hide resolved
@alliepiper alliepiper removed testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). testing: gpuCI in progress Started gpuCI testing. labels Sep 29, 2021
@bjude
Copy link
Contributor Author

bjude commented Sep 29, 2021

Damn, that one didnt show up with MSVC, they seem to be a bit inconsistent with when the signed vs unsigned warning triggers.

I'll get it fixed up today

@alliepiper
Copy link
Collaborator

Thanks!

DVS CL: 30482082

run tests

@alliepiper alliepiper merged commit e99e10b into NVIDIA:main Oct 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sequence() should not convert the index to T
3 participants