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

Use of std::invoke to run the body in algorithms and graph nodes #880

Merged
merged 22 commits into from May 17, 2023

Conversation

kboyarinov
Copy link
Contributor

Description

Change the execution of the body in TBB algorithms and Flow Graph nodes to use std::invoke instead of operator().
It allows to pass pointers to non-static member functions and non-static member objects as a body.
Affected components:

  • parallel_for
  • parallel_reduce
  • parallel_deterministic_reduce
  • parallel_for_each
  • parallel_scan
  • parallel_pipeline
  • function_node
  • multifunction_node
  • async_node
  • sequencer_node
  • join_node with key_matching policy

Fixes # - issue number(s) if exists

  • - git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details)

Type of change

Choose one or multiple, leave empty if none of the other choices apply

Add a respective label(s) to PR if you have permissions

  • bug fix - change that fixes an issue
  • new feature - change that adds functionality
  • tests - change in tests
  • infrastructure - change in infrastructure and CI
  • documentation - documentation update

Tests

  • added - required for new features and some bug fixes
  • not needed

Documentation

  • updated in # - add PR number
  • needs to be updated
  • not needed

Breaks backward compatibility

  • Yes
  • No
  • Unknown

Notify the following users

List users with @ to send notifications

Other information

Copy link

@vossmjp vossmjp left a comment

Choose a reason for hiding this comment

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

Partial review. So far have looked at _utils.h, parallel_for.h, parallel_for_each.h, parallel_scan.h, _pipeline_filters.h and parallel_reduce.h. Mostly looks good to me. A couple of small comments/questions. Still need to look at tests, conformance tests & flow graph.

include/oneapi/tbb/detail/_pipeline_filters.h Show resolved Hide resolved
include/oneapi/tbb/parallel_for.h Show resolved Hide resolved
Copy link
Contributor

@omalyshe omalyshe left a comment

Choose a reason for hiding this comment

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

Fixes needed.

include/oneapi/tbb/parallel_scan.h Show resolved Hide resolved
test/tbb/test_task.cpp Show resolved Hide resolved
Copy link

@vossmjp vossmjp left a comment

Choose a reason for hiding this comment

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

The flow graph modifications look ok to me. Made a couple of comments to make sure I understand why certain invocation points are excluded.

vossmjp
vossmjp previously approved these changes May 17, 2023
Copy link

@vossmjp vossmjp left a comment

Choose a reason for hiding this comment

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

Did a quick look through tests and they look ok to me.

@kboyarinov kboyarinov merged commit a088cfa into master May 17, 2023
17 of 19 checks passed
@kboyarinov kboyarinov deleted the dev/kboyarinov/invoke_semantics branch May 17, 2023 16:29
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

4 participants