Use placeholder expressions

[mfrancis95]

Use placeholder expression in thrust::count

Use placeholder expression in thrust::find

Use placeholder expression in thrust::mismatch

Use placeholder expression in thrust::sequence

Remove sequence_functor

Use placeholder expression in thrust::remove

Use placeholder expression in thrust::find

Use placeholder expression in thrust::replace

Use placeholder expression in thrust::replace

Use placeholder expression in thrust::replace_copy

[alliepiper]

Thanks @mfrancis95.

Following up the conversation in #1169, the default cpp.cuda.cpp14 config builds in 17:05 on my machine, which is not noticeably different than before.

I found a couple missing headers while building some of the non-CUDA configurations, these should be fixed in the commit I just pushed.

This LGTM, I'll start internal CI soon.

[mfrancis95]

Thanks for handling the missing headers. My goal was to try to remove the need for equal_to_value entirely, but there were a couple of cases where replacing it with placeholders wouldn't compile.

[alliepiper]

There have been some issues reported when using placeholders (e.g. #1178) that we can better address now that C++03 has been dropped. I suspect it was a similar issue?

In any case, it was good to find out that the placeholder expression templates don't add much compilation overhead. Maybe someday we can switch over completely.

Started internal CI in CL 28521254.

[alliepiper]

@jaredhoberock This looks good to me, and tests are passing. Have your concerns (from #1169) been addressed?

[mfrancis95]

When I get some time on this again I'll share the errors I got when I attempted to replace some instances of equal_to_value with placeholders.

Luckily there aren't many instances of that left, so it hopefully wouldn't take too much to remove that functor entirely.

[mfrancis95]

Here's an example of a change that fails:

diff --git a/thrust/system/cuda/detail/remove.h b/thrust/system/cuda/detail/remove.h
index c590a1ad..c8560148 100644
--- a/thrust/system/cuda/detail/remove.h
+++ b/thrust/system/cuda/detail/remove.h
@@ -35,7 +35,7 @@ namespace thrust
 namespace cuda_cub {
 
 // in-place

 template <class Derived,
           class InputIt,
           class StencilIt,
@@ -124,8 +124,8 @@ remove_copy(execution_policy<Derived> &policy,
             OutputIt                   result,
             const T &                  value)
 {
-  thrust::detail::equal_to_value<T> pred(value);
-  return cuda_cub::remove_copy_if(policy, first, last, result, pred);
+  using thrust::placeholders::_1;
+  return cuda_cub::remove_copy_if(policy, first, last, result, _1 == value);
 }
 
 }    // namespace cuda_cub

[jaredhoberock]

@allisonvacanti Thanks for verifying that the build time didn't change.

My only remaining concern is that the ad hoc functors that these placeholder expressions replace should be eliminated, if possible.

It looks like these changes eliminate sequence_functor, is it possible to eliminate equal_to_value as well?

Also, shouldn't these changes make some of the #includes used by these implementations superfluous? Where is equal_to_value defined?

[mfrancis95]

@jaredhoberock see the snippet above. I think it's possible to eliminate that functor but it'll take some time to understand why it can't be substituted in a few places currently and how to fix it. Maybe it is related to #1178?

[jaredhoberock]

@mfrancis95 Thanks. Let's not worry about equal_to_value in this issue, then.

I'd still like to eliminate #include <thrust/detail/internal_functional.inl> in these files, where it is possible to do so. This is the header which defines these ad hoc functors.

For example, I think eliminating the one at the top of thrust/system/detail/generic/replace.inl ought to be possible.

[mfrancis95]

Hi @jaredhoberock. After going through the files, thrust/system/detail/generic/replace.inl was the only file where I was able to remove that include. Other files that I modified still had a couple of instances where they were using other internal functors such as thrust::detail::not1.

The commit is going to be pushed up momentarily.

[mfrancis95]

@allisonvacanti there's been a new commit pushed up to this branch. I would assume I did all that can be done for this particular wave of changes for now.

I'd want to see #1178 fixed before coming back and doing more.

[alliepiper]

That sounds good to me. I'll restart the testing and land this if all goes well.

[alliepiper]

Rebased and squashed in last push.

Running CI under 28707351

[alliepiper]

CI looks good, I'll merge this soon.