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

Preparing zip_iterator and tuple_of_iterator_references for variadic tuple #1311

Closed

Conversation

andrewcorrigan
Copy link
Contributor

@andrewcorrigan andrewcorrigan commented Oct 12, 2020

Depends on #1310. This reduces dependence of zip_iterator on implementation details of thrust::tuple, such as head_type, tail_type, and null_type.

The reason I submitted this as a separate PR is the following. Unfortunately, certain implementation details remain very inconvenient to workaround. Therefore, I introduced a guard THRUST_VARIADIC_TUPLE, which is obviously disabled. Upon a future variadic thrust::tuple (regardless of whether it's from libcudacxx, std::tuple, jaredhoberock/tuple, etc.), the code guarded by THRUST_VARIADIC_TUPLE can be activated, while the #else branch code can be removed. The code guarded by THRUST_VARIADIC_TUPLE works. I have tested it extensively over the past 5 years on my variadic branch.

With -DTHRUST_DEVICE_SYSTEM=CPP:

100% tests passed, 0 tests failed out of 151

Total Test time (real) = 109.96 sec

@alliepiper alliepiper added this to the 1.11.0 milestone Oct 12, 2020
@alliepiper
Copy link
Collaborator

The split between these PRs makes sense to me. I'll focus on getting the other one landed, and probably hold off on adding this until we have the new tuple in-place and can remove the preprocessor guards.

@alliepiper alliepiper modified the milestones: 1.11.0, Backlog Oct 12, 2020
@alliepiper alliepiper added the blocked Cannot make progress. label Oct 12, 2020
@alliepiper
Copy link
Collaborator

For bookkeeping, this PR is blocked on NVIDIA/cccl#742.

@andrewcorrigan
Copy link
Contributor Author

In that case, since it's blocked anyway, I'll redo the commit without the guard. Then, once NVIDIA/cccl#742 is taken care of, it can be merged as-is. I also have code ready to go for tuple_of_iterator_references, which is directly related since it used by zip_iterator and similarly blocked (related to incompatibility between non-variadic and variadic tuple constructors). I would like to build that change also into this PR.

FYI, in case you're interested in seeing these changes in action, I created a new variadic tuple branch. I tried building and running all the tests. They succeeded with one exception, that I disabled.

@andrewcorrigan andrewcorrigan force-pushed the variadic-zip-iterator branch 2 times, most recently from 5469012 to c203d48 Compare October 12, 2020 23:53
@andrewcorrigan andrewcorrigan changed the title Preparing zip_iterator for variadic tuple Preparing zip_iterator and tuple_of_iterator_references for variadic tuple Oct 13, 2020
@andrewcorrigan andrewcorrigan force-pushed the variadic-zip-iterator branch 2 times, most recently from 33518ad to 28a4267 Compare October 13, 2020 17:21
@andrewcorrigan
Copy link
Contributor Author

The last commit fixes the one test that I mentioned had failed. All tests now pass using these changes on my branch.

@andrewcorrigan
Copy link
Contributor Author

@allisonvacanti It looks like the reason for the incompatibility was #1314. I was not expecting such a simple fix. With #1314 merged, I think that this PR could be ready to go as soon as #1310 is merged.

Would it make more sense to add these commits to #1310 and close this PR? Or should I keep it separate?

@alliepiper
Copy link
Collaborator

Ahh, good catch, that makes some amount of sense.

It'd probably be simpler to just combine the PRs and remove the workaround. We have some time -- our internal CI is currently unusable so I won't be able to move this forward until sometime next week at the earliest anyway.

@alliepiper alliepiper removed this from the Backlog milestone Apr 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocked Cannot make progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants