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

partial fix for iterator traits and tags #1685

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

Conversation

ericniebler
Copy link
Collaborator

The symptoms of this problem are described in NVIDIA/cccl#705. Really, there are two problems with the way Thrust's iterator tags are currently defined:

  1. The *_device_* iterator tags are erroneously convertible the *_host_* tags. That's because the *_host_* tags are simply type aliases for the standard iterator tags, which the *_device_* tags inherit from.

  2. Because of the way the device iterator tags are defined (with iterator_category_with_system_and_traversal) there is no relationship between the different device tags. We would expect (and much of the code does) that thrust::forward_device_iterator_tag is convertible to thrust::input_device_iterator_tag, but that is sadly not true.

The two traits, iterator_category_to_system and iterator_category_to_traversal, are first checking whether the category is convertible to a host iterator tag. That is trivially true even for the device tags because of (1); as a result, device tags appear to have the "host" system, which is incorrect.

This PR offers the simplest, least intrusive, and sadly partial fix to the problem reported in NVIDIA/cccl#705. It changes the two traits so that they first check for convertibility to the device tag. This gets the answer correctly in more cases than currently.

I think I know how to fix this properly, but not without potentially breaking some code, so I'm submitting this PR now as a stop-gap.

@ericniebler ericniebler added type: bug: functional Does not work as intended. P1: should have Necessary, but not critical. area: iterators Related to Thrust's fancy iterators. labels May 10, 2022
@ericniebler ericniebler requested a review from alliepiper May 10, 2022 23:04
@ericniebler
Copy link
Collaborator Author

run tests

@alliepiper alliepiper added this to the 2.0.0 milestone May 11, 2022
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.

The fix looks safe to me, but there are some style/test issues to clean up before this goes in.

@ericniebler
Copy link
Collaborator Author

@allisonvacanti I see you tagged this with "2.0" milestone, but this is a change that is safe to make in the 1.X series, IMO.

@alliepiper
Copy link
Collaborator

alliepiper commented May 12, 2022

this is a change that is safe to make in the 1.X series, IMO.

It would be, but the RC is already out. We only restart the RC process to add fixes for regressions.

@ericniebler
Copy link
Collaborator Author

run tests

@ericniebler
Copy link
Collaborator Author

ericniebler commented May 14, 2022

I suppose if an iterator had any_system_tag as its system tag, then this diff would change its reported system from host to device. 🤔

@allisonvacanti, do any iterators use any_system_tag?

@alliepiper
Copy link
Collaborator

do any iterators use any_system_tag?

Yes -- this tag is used by iterators that aren't backed by actual memory (e.g thrust::counting_iterator, thrust::constant_iterator, etc, as well as several in CUB).

I suppose if an iterator had any_system_tag as its system tag, then this diff would change its reported system from host to device. 🤔

This should be fine. The device system is typically faster than the host system, so this is the preferred behavior. If an algorithm takes an any_system iterator and also a host_system iterator (e.g. thrust::copy(any_system_it, any_system_it_end, host_system_it_begin)) and requires the algorithm to run on host, it should still work since we check the systems of all iterators while dispatching.

@alliepiper alliepiper modified the milestones: 2.0.0, 2.1.0 Jul 25, 2022
@miscco
Copy link
Collaborator

miscco commented Feb 21, 2023

@ericniebler @allisonvacanti

Is this ready to merge?

@ericniebler
Copy link
Collaborator Author

Is this ready to merge?

I ... think so? I can rebase, retest and commit if all looks good.

@ericniebler
Copy link
Collaborator Author

run tests

@miscco
Copy link
Collaborator

miscco commented Feb 22, 2023

Hm that one is strange:

/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/numeric_traits.h(70): error: qualified name is not allowed

/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/numeric_traits.h(72): error: expected a "("

/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/numeric_traits.h(72): error: expected a type specifier

/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/numeric_traits.h(72): error: expected a ")"

/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/numeric_traits.h(76): error: expected a "("

/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/numeric_traits.h(76): error: expected a type specifier

/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/numeric_traits.h(77): error: expected a ";"

/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/numeric_traits.h(78): error: expected a "("

/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/numeric_traits.h(78): error: expected a ";"

/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/numeric_traits.h(88): error: class template "__gnu_cxx::__numeric_traits_integer<_Value>" has no member "__is_signed"

There is nothing in this PR that could have triggered this

@gevtushenko
Copy link
Collaborator

@miscco we have wait till #1866 is merged.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: iterators Related to Thrust's fancy iterators. P1: should have Necessary, but not critical. type: bug: functional Does not work as intended.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants