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

Implement unified ETLS without TBB and use it for all host backends #2048

Merged
merged 17 commits into from
Feb 5, 2025

Conversation

danhoeflinger
Copy link
Contributor

@danhoeflinger danhoeflinger commented Feb 5, 2025

tbb/enumerable_thread_specific.h includes an indirect inclusion of windows.h on windows.
This is undesirable, and therefore we would like to avoid it. Without enumerable_thread_specific to depend upon in TBB, we can instead unify all implementations into a shared one which uses a pair of helpers:
__get_thread_num()
and
__get_num_threads()
which are passed to the shared implementation from the parallel backends as template arguments during creation in the make function.

We implement a make function for each backend to properly pass in the appropriate helper functions for the parallel backend. We no longer need to bury the __enumerable_thread_local_storage in a __details namespace, since all backends could in theory directly use the constructor if they fill out all the appropriate template arguments. Variadic Args... are present in the type itself for all backends including TBB now. However, make functions are beneficial to keep verbosity out of the local usage of __enumerable_thread_local_storage.


Sadly, I don't see a way to automatically pick up the appropriate helper routines from the parallel backend from within include/oneapi/dpl/pstl/parallel_backend_utils.h because of a circular dependency without adding another utils header.

We could consider adding another header include/oneapi/dpl/pstl/parallel_unified_utils.h which depends on the parallel backend rather than vice-versa. This could allow us to depend upon a generic __par_backend:: call for the two helpers. We could also unify all make functions into this header. The downside to this would be that we sacrifice the ability to use ETLS from the backends themselves, which we dont do now, but we may want to do in the future.

@danhoeflinger danhoeflinger marked this pull request as ready for review February 5, 2025 18:03
Signed-off-by: Dan Hoeflinger <[email protected]>
move back into __detail namespace

Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
@SergeyKopienko
Copy link
Contributor

@danhoeflinger, I haven't any additional questions or comments from my side.

Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Copy link
Contributor

@mmichel11 mmichel11 left a comment

Choose a reason for hiding this comment

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

The approach here LGTM.

include/oneapi/dpl/pstl/parallel_backend_utils.h Outdated Show resolved Hide resolved
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
akukanov
akukanov previously approved these changes Feb 5, 2025
Copy link
Contributor

@akukanov akukanov left a comment

Choose a reason for hiding this comment

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

Looks good.

mmichel11
mmichel11 previously approved these changes Feb 5, 2025
Copy link
Contributor

@mmichel11 mmichel11 left a comment

Choose a reason for hiding this comment

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

LGTM

caused an issue on cl2017

Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger danhoeflinger dismissed stale reviews from mmichel11 and akukanov via 3198af3 February 5, 2025 21:04
@danhoeflinger
Copy link
Contributor Author

I had to revert CRTP implementation due to an issue on CL2017. Back to passing callables details from the individual parallel backends. Otherwise, nothing has changed since approval. CI run is going now.

Copy link
Contributor

@mmichel11 mmichel11 left a comment

Choose a reason for hiding this comment

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

Reapproving

@danhoeflinger danhoeflinger merged commit 4a9d3f4 into main Feb 5, 2025
22 checks passed
@danhoeflinger danhoeflinger deleted the dev/dhoeflin/ets_without_tbb_tes branch February 5, 2025 21:58
@akukanov akukanov restored the dev/dhoeflin/ets_without_tbb_tes branch February 5, 2025 21:58
@akukanov akukanov deleted the dev/dhoeflin/ets_without_tbb_tes branch February 6, 2025 00:20
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.

4 participants