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.

Sign up for GitHub

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

Restore compilaltion with tbb 2017u7 #3007

Merged
merged 3 commits into from
Dec 1, 2020
Merged

Restore compilaltion with tbb 2017u7 #3007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb3bf2d
Restore compilaltion with tbb 2017u7
AlexPeskov Nov 6, 2020
0ca94c5
Fix unsupported arg for tbb deterministic_reduce
AlexPeskov Nov 9, 2020
9b82174
Merge branch 'master' into ap/old-tbb-compilation
Nov 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions inference-engine/cmake/ie_parallel.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ function(set_ie_threading_interface_for TARGET_NAME)
find_package(TBB COMPONENTS tbb tbbmalloc)
set("TBB_FOUND" ${TBB_FOUND} PARENT_SCOPE)
set("TBB_IMPORTED_TARGETS" ${TBB_IMPORTED_TARGETS} PARENT_SCOPE)
set("TBB_VERSION" ${TBB_VERSION} PARENT_SCOPE)
Copy link
Contributor

Choose a reason for hiding this comment

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

may we need to put them in cache? In this case we don't need play with PARENT_SCOPE in other files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Global cache is not always good idea. Particularly for this case we have a problem multiple TBB::tbb target definition only when call file package from one cmake script. If try it form different places cmake keeps this TBB import targets locally and there are no conflicts.

So if we change TBB_FOUND to cache it will break other places where TBB is used.

Other concern. As you know, cache are stored separately, and will be reused for next cmake reload. For that case we will miss TBB target forever till next reset cache and reload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other question WHY did you move find_package inside a function? Was it request from @mryzhov?

Previously it was exported in root cmake file and will be available for each subfolders in OV project. But not each subfolder try to create it own version of tbb import.

Copy link
Contributor

Choose a reason for hiding this comment

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

it was requested by @alalek because for 3rd party component we don't need to find TBB since TBB it's a private IE dependency.

E.g. when OpenCV is build against IE, InferenceEngineConfig.cmake finds TBB, but does not use it (it can use it only if set_threading_interface is called for some targets)

Copy link

Choose a reason for hiding this comment

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

To avoid issues with function's scope it makes sense to convert it into a macro (or split into a macro part with find_package() and a function with internal logic).

Copy link
Contributor

Choose a reason for hiding this comment

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

during skype discussion we also agreed with @AlexPeskov that we also need to find TBB only in IE paths (and we need to set TBB_DIR / TBBROOT for that before calling ie_parallel.cmake), because currently find TBB can find any TBB on the system, because nobody sets proper location when we use prebuild OpenVINO package.

Copy link
Contributor

Choose a reason for hiding this comment

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

@AlexPeskov are you going to apply changes we agreed?

if (TBB_FOUND)
if (TBB_VERSION VERSION_LESS 2020)
ext_message(WARNING "TBB version is less than OpenVINO recommends to use.\
Expand Down
15 changes: 12 additions & 3 deletions inference-engine/include/ie_parallel.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@ inline int parallel_get_env_threads() {
}
#if IE_THREAD == IE_THREAD_TBB
#define PARTITIONING , tbb::static_partitioner()

// The TBB version less than 2018u1 has no static_partitioner argument for
// tbb::parallel_deterministic_reduce. So will fallback to non deterministic version.
#if (TBB_INTERFACE_VERSION >= 10001)
#define _TBB_REDUCE_FUNC tbb::parallel_deterministic_reduce
#else
#define _TBB_REDUCE_FUNC tbb::parallel_reduce
#endif

#else
#define PARTITIONING
#endif
Expand Down Expand Up @@ -186,7 +195,7 @@ void parallel_sort(I begin, I end, const F& comparator) {
template <typename T0, typename R, typename F>
R parallel_sum(const T0& D0, const R& input, const F& func) {
#if (IE_THREAD == IE_THREAD_TBB || IE_THREAD == IE_THREAD_TBB_AUTO)
return tbb::parallel_deterministic_reduce(
return _TBB_REDUCE_FUNC(
tbb::blocked_range<T0>(0, D0), input,
[&](const tbb::blocked_range<T0>& r, R init) -> R {
R sum = init;
Expand Down Expand Up @@ -218,7 +227,7 @@ R parallel_sum(const T0& D0, const R& input, const F& func) {
template <typename T0, typename T1, typename R, typename F>
R parallel_sum2d(const T0& D0, const T1& D1, const R& input, const F& func) {
#if (IE_THREAD == IE_THREAD_TBB || IE_THREAD == IE_THREAD_TBB_AUTO)
return tbb::parallel_deterministic_reduce(
return _TBB_REDUCE_FUNC(
tbb::blocked_range2d<T0, T1>(0, D0, 0, D1), input,
[&](const tbb::blocked_range2d<T0, T1>& r, R init) -> R {
R sum = init;
Expand Down Expand Up @@ -257,7 +266,7 @@ R parallel_sum2d(const T0& D0, const T1& D1, const R& input, const F& func) {
template <typename T0, typename T1, typename T2, typename R, typename F>
R parallel_sum3d(const T0& D0, const T1& D1, const T2& D2, const R& input, const F& func) {
#if (IE_THREAD == IE_THREAD_TBB || IE_THREAD == IE_THREAD_TBB_AUTO)
return tbb::parallel_deterministic_reduce(
return _TBB_REDUCE_FUNC(
tbb::blocked_range3d<T0, T1, T2>(0, D0, 0, D1, 0, D2), input,
[&](const tbb::blocked_range3d<T0, T1, T2>& r, R init) -> R {
R sum = init;
Expand Down
7 changes: 7 additions & 0 deletions inference-engine/tests/functional/inference_engine/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,13 @@ function(ie_headers_compilation_with_custom_flags)
# To include TBB headers as system
set_ie_threading_interface_for(${target_name})

# To avoid further TBB find_package action in next call of this function. Some version of TBB
# has an issue with cmake config which lead to fail in case of multiple call of find_package
# from one cmake script file.
set("TBB_FOUND" ${TBB_FOUND} PARENT_SCOPE)
set("TBB_IMPORTED_TARGETS" ${TBB_IMPORTED_TARGETS} PARENT_SCOPE)
set("TBB_VERSION" ${TBB_VERSION} PARENT_SCOPE)

set_target_properties(${target_name} PROPERTIES
CXX_STANDARD ${IE_TEST_CXX_STANDARD}
CXX_STANDARD_REQUIRED OFF)
Expand Down