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

Jump to bottom

Restore compilaltion with tbb 2017u7 #3007

Merged
merged 3 commits into from
Dec 1, 2020
Merged

Restore compilaltion with tbb 2017u7 #3007

merged 3 commits into from
Dec 1, 2020

Conversation

AlexPeskov
Copy link
Contributor

Previous PR 2966 just broke build with retro TBB.
This patch should fix it.

@AlexPeskov AlexPeskov requested review from a team November 6, 2020 11:13
@AlexPeskov
Copy link
Contributor Author

@asuhov could you please check tbb 2017 build with this patch?

@AlexPeskov AlexPeskov added the category: inference OpenVINO Runtime library - Inference label Nov 6, 2020
ilya-lavrenov
ilya-lavrenov reviewed Nov 6, 2020
View reviewed changes
inference-engine/cmake/ie_parallel.cmake
@@ -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?

@openvino-pushbot openvino-pushbot added the category: IE Tests OpenVINO Test: plugins and common label Nov 6, 2020
@AlexPeskov AlexPeskov force-pushed the ap/old-tbb-compilation branch from 99d162e to c97888d Compare November 6, 2020 12:24
@asuhov
Copy link
Contributor

asuhov commented Nov 6, 2020

@AlexPeskov IE build with TBB 2017 failed, please check this log:
https://openvino-ci.intel.com/blue/rest/organizations/jenkins/pipelines/private-ci/pipelines/ie/pipelines/build-linux-ubuntu18/runs/36931/nodes/183/log/

@AlexPeskov
Copy link
Contributor Author

@AlexPeskov IE build with TBB 2017 failed, please check this log:
https://openvino-ci.intel.com/blue/rest/organizations/jenkins/pipelines/private-ci/pipelines/ie/pipelines/build-linux-ubuntu18/runs/36931/nodes/183/log/

@asuhov could you please rerun this job. It missed last fix for reduce operation.

@AlexPeskov AlexPeskov force-pushed the ap/old-tbb-compilation branch from 22339d6 to 0ca94c5 Compare November 27, 2020 18:31
@ilya-lavrenov ilya-lavrenov added this to the 2021.3 milestone Dec 1, 2020
@ilya-lavrenov ilya-lavrenov merged commit f6dcf45 into openvinotoolkit:master Dec 1, 2020
ilya-lavrenov pushed a commit to ilya-lavrenov/openvino that referenced this pull request Dec 1, 2020
@AlexPeskov @ilya-lavrenov
Restore compilaltion with tbb 2017u7 (openvinotoolkit#3007)
2c17b38 
* Restore compilaltion with tbb 2017u7

Signed-off-by: Alexander Peskov <[email protected]>

* Fix unsupported arg for tbb deterministic_reduce

Signed-off-by: Alexander Peskov <[email protected]>

Co-authored-by: Alexander Zhogov <[email protected]>
@ilya-lavrenov ilya-lavrenov mentioned this pull request Dec 1, 2020
Merged
ilya-lavrenov added a commit that referenced this pull request Dec 2, 2020
@ilya-lavrenov @AlexPeskov
Restore compilaltion with tbb 2017u7 (#3007) (#3426)
f730bf9 
* Restore compilaltion with tbb 2017u7

Signed-off-by: Alexander Peskov <[email protected]>

* Fix unsupported arg for tbb deterministic_reduce

Signed-off-by: Alexander Peskov <[email protected]>

Co-authored-by: Alexander Zhogov <[email protected]>

Co-authored-by: Alexander Peskov <[email protected]>
Co-authored-by: Alexander Zhogov <[email protected]>
evolosen pushed a commit to evolosen/openvino that referenced this pull request Dec 3, 2020
@AlexPeskov @evolosen
Restore compilaltion with tbb 2017u7 (openvinotoolkit#3007)
62ce3fa 
* Restore compilaltion with tbb 2017u7

Signed-off-by: Alexander Peskov <[email protected]>

* Fix unsupported arg for tbb deterministic_reduce

Signed-off-by: Alexander Peskov <[email protected]>

Co-authored-by: Alexander Zhogov <[email protected]>
mryzhov pushed a commit to mryzhov/openvino that referenced this pull request Dec 11, 2020
@AlexPeskov @mryzhov
Restore compilaltion with tbb 2017u7 (openvinotoolkit#3007)
a02343a 
* Restore compilaltion with tbb 2017u7

Signed-off-by: Alexander Peskov <[email protected]>

* Fix unsupported arg for tbb deterministic_reduce

Signed-off-by: Alexander Peskov <[email protected]>

Co-authored-by: Alexander Zhogov <[email protected]>
mryzhov pushed a commit to mryzhov/openvino that referenced this pull request Dec 16, 2020
@AlexPeskov @mryzhov
Restore compilaltion with tbb 2017u7 (openvinotoolkit#3007)
3b28b42 
* Restore compilaltion with tbb 2017u7

Signed-off-by: Alexander Peskov <[email protected]>

* Fix unsupported arg for tbb deterministic_reduce

Signed-off-by: Alexander Peskov <[email protected]>

Co-authored-by: Alexander Zhogov <[email protected]>
mryzhov pushed a commit to mryzhov/openvino that referenced this pull request Jan 14, 2021
@AlexPeskov @mryzhov
Restore compilaltion with tbb 2017u7 (openvinotoolkit#3007)
1761dad 
* Restore compilaltion with tbb 2017u7

Signed-off-by: Alexander Peskov <[email protected]>

* Fix unsupported arg for tbb deterministic_reduce

Signed-off-by: Alexander Peskov <[email protected]>

Co-authored-by: Alexander Zhogov <[email protected]>
jiwaszki pushed a commit to akuporos/openvino that referenced this pull request Jan 15, 2021
@AlexPeskov
Restore compilaltion with tbb 2017u7 (openvinotoolkit#3007)
3f39fbc 
* Restore compilaltion with tbb 2017u7

Signed-off-by: Alexander Peskov <[email protected]>

* Fix unsupported arg for tbb deterministic_reduce

Signed-off-by: Alexander Peskov <[email protected]>

Co-authored-by: Alexander Zhogov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alalek alalek alalek left review comments

@ilya-lavrenov ilya-lavrenov ilya-lavrenov approved these changes

Assignees

@ilya-lavrenov ilya-lavrenov

Labels
category: IE Tests OpenVINO Test: plugins and common category: inference OpenVINO Runtime library - Inference
Projects
None yet
Milestone
2021.3
Development

Successfully merging this pull request may close these issues.
5 participants
@AlexPeskov @asuhov @ilya-lavrenov @alalek @openvino-pushbot