Replace thrust::tuple implementation with cuda::std::tuple
#262
Conversation
miscco
requested review from
alliepiper and
gevtushenko
and removed request for
a team
miscco
added
feature request
miscco
force-pushed
the
replace_thrust_tuple
branch
from
4211273 to
2af6a61
Compare
miscco
force-pushed
the
replace_thrust_tuple
branch
4 times, most recently
from
4aadf81 to
1472180
Compare
miscco
force-pushed
the
replace_thrust_tuple
branch
from
0cb8648 to
fea0a59
Compare
miscco
force-pushed
the
replace_thrust_tuple
branch
3 times, most recently
from
5a93a81 to
93b0eec
Compare
miscco
force-pushed
the
replace_thrust_tuple
branch
4 times, most recently
from
dccc4ec to
9d0ff36
Compare
Our old implementation of `tuple` has a lot of limitations. So rather than using the old homegrown version reuse `cuda::std::tuple`
miscco
force-pushed
the
replace_thrust_tuple
branch
from
9d0ff36 to
471b220
Compare
NVIDIA#262)" This reverts commit 61c1fd0.
thrust/thrust/tuple.h
Outdated
| template<class T0, class T1, class T2, class T3, class T4, class T5, class T6, class T7> | ||
| struct tuple_size<tuple<T0, T1, T2, T3, T4, T5, T6, T7, THRUST_NS_QUALIFIER::null_type>> : tuple_size<tuple<T0, T1, T2, T3, T4, T5, T6, T7>> {}; |
There was a problem hiding this comment.
A bit late to the party, but: why does this only go up to 8 non-null elements (0-7)? Shouldn't this go up to 9 (0-8)? AFAICT the old tuple implementation supported up to 10 items, so shouldn't 9 non-null types plus 1 null type be handled here?
If true, I think this was unfortunately hidden by the testing added in 471b220 falling one element short, as well. There's no test for the case of 9 non-null types followed by thrust::null_type having a tuple size of 9. I believe this would currently (incorrectly) produce the standard tuple size of 10.
There was a problem hiding this comment.
Thinking further, I'm guessing that these specializations were added for compatibility with existing code that fully parameterizes tuple? In that case, since they would be specifiying all 10 parameters and all of these specializations only specify 9, none these specializations would match.
When I made my initial comment, I thought that, while only specifying 9 parameters for all of these specializations seemed odd, it must have been ok because the template parameter defaults specified for tuple would provide thrust::null_type for the 10th. But that was only true of the old tuple definition, in which case this compatibility layer wouldn't be necessary anyway.
tl;dr: In addition to adding the 9 + 1 specialization, I think that all existing specializations (and tests) should be extended from 9 to 10 template parameters.
There was a problem hiding this comment.
😱 Seems like some of them did not make it
There was a problem hiding this comment.
But I believe the analysis is not correct, we only need to pass 10 - 1 == 9 arguments.
Note that we always append a null_type which is the missing one.
Our old implementation of
tuplehas a lot of limitations.So rather than using the old homegrown version reuse
cuda::std::tuple