Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PoC: For Each (Range) Index #552

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from
Draft

Conversation

samayala22
Copy link

Proof of concept for feature proposed in #551

A few notes:

  • I firmly believe that increment should not be part of the API and the called function should have the responsibility to handle any sort of increment. The fact that TBB doesn't have an increment argument supports my point.
  • I don't understand why begin, end (and the removed step) are implemented as different template arguments ? They should be the same integral types ? Am I missing something ?
  • This is a PoC so the code is not intended to be merged as is.

Minimal example code of the new functionality:

#include <taskflow/taskflow.hpp>
#include <taskflow/algorithm/for_each.hpp>

#include <iostream>
#include <mutex>

int main() {
    tf::Executor executor;
    tf::Taskflow taskflow;
    std::mutex mutex;

    auto init = taskflow.placeholder();
    auto body_index = taskflow.for_each_index(0, 10, [&](int i) {
        std::scoped_lock lock(mutex);
        std::cout << "Hello from task " << i << std::endl;
    });

    auto body_range = taskflow.for_each_index(0, 1000, [&](int b, int e) {
        std::scoped_lock lock(mutex);
        std::cout << "Hello from task range [" << b << ", " << e << ")" << std::endl;
    });

    init.precede(body_index);
    body_index.precede(body_range);

    executor.run(taskflow).wait();
    
    return 0;
}

@samayala22 samayala22 marked this pull request as draft February 8, 2024 16:15
idk this is messy
@tsung-wei-huang
Copy link
Member

@samayala22 The reason why input, output iterator types are templated is that applications may mix the use of stateful iterators, constant, or non-stateful iterators. For example:

std::vector<int>::iterator itr;
tf.for_each(1, std::ref(itr), ...)
tf.for_each(1, 100, ...);
tf.for_each(std::ref(itr), 100, ...)
...

Copy link
Contributor

@bangerth bangerth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a topic of interest to me. So here are a couple of comments.

Comment on lines 84 to +86
// Function: make_for_each_index_task
template <typename B, typename E, typename S, typename C, typename P = DefaultPartitioner>
TF_FORCE_INLINE auto make_for_each_index_task(B b, E e, S s, C c, P part = P()){
template <typename T, typename C, typename P = DefaultPartitioner>
TF_FORCE_INLINE auto make_for_each_index_task(T b, T e, C c, P part = P()){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment in the PR is specifically about this part of the patch. The begin and end iterators, b and e may sometimes be of different types and I think it is useful to retain the ability to have different types as was there before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separately, though, I think it is important to keep the original interface for backward compatibility. Perhaps you want to overload the function by providing a second version that takes the two-argument function (instead of modifying the existing function)? You can use std::enable_if_t to ensure the right one gets called.


static_assert(std::is_integral<T_t>::value, "Begin and end values must be an integral type.");
static_assert(
std::disjunction<is_index_func<T_t, C>, is_range_func<T_t, C>>::value,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be easier to read as follows :-)

Suggested change
std::disjunction<is_index_func<T_t, C>, is_range_func<T_t, C>>::value,
is_index_func<T_t, C>::value || is_range_func<T_t, C>::value,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants