-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
Update TBB interface and allow using external lib #2257
Conversation
Define TBB_INTERFACE_NEW to use the new interface of Intel/OneAPI TBB system library. This can be added in the user-defined variables in `~/.config/stan/make.local` or `make/local`. TBB updated functionality: https://software.intel.com/content/www/us/en/develop/articles/tbb-revamp.html TBB release notes and backward compatibility: https://software.intel.com/content/www/us/en/develop/articles/intel-oneapi-threading-building-blocks-release-notes.html Intel OneAPI release notes: https://software.intel.com/content/www/us/en/develop/articles/intel-oneapi-toolkit-release-notes.html Note that Intel OneAPI automatically sets the required environment variables for TBB. Signed-off-by: Hamada S. Badr <hamada.s.badr@gmail.com>
…4.1 (tags/RELEASE_600/final)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @hsbadr! A couple of minor questions and comments.
Also, do we need to branch in our sources on a macro like TBB_INTERFACE_NEW? I mean what is new and what old changes tomorrow. Instead I would prefer to branch based on a version number. The TBB is likely exposing some TBB version string with their headers in some of their defines. Couldn't we use that to branch? |
Good call. Agree. |
@wds15 @rok-cesnovar Good point. We can do it based on the version number, but |
Oh, that's unfortunate then. I am leaning slightly towards |
The problem with using |
Oh, but there is a slight difference there. STAN_OPENCL=true means use OpenCL backend, STAN_MPI=true means use MPI backend and STAN_THREADS means use TBB for threading-enabled functions. This is why I don't feel like STAN_TBB is the best name here as it would suggest "use TBB" when it's actually "use the system installed TBB". TBB_USE_SYSTEM_LIB would be my choice, I am just not sure what to do for branching in the header file. Let me take a look and get back to you tomorrow. |
Now, |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Reading through this PR suggests that we do two things at the same time: external TBB and new TBB... but can this be separate? I mean, maybe a user wants to use an old TBB of the system with stan-math or we may want to bump the TBB version in stan-math. Will these patches then still work as expected? |
@wds15 Exactly! That's what the last commit does. And, yes, this will work in any combinations of Here, if |
This looks good to me, I would maybe change the INTERFACE NEW to TBB_USE_GLOBAL_CONTROL. After we switch to a TBB with this new interface this will be the default and only setting. @wds15 does that sound good for you? |
@rok-cesnovar Also, I'd like to replace I personally prefer it to be modular for any external library (i.e., allow the user to use internal or external libraries) unless you make custom changes to its source code. In the case of TBB, there's no need to build the internal source code if the library and its headers are available in the system. |
Ok, then lets leave it at the new interface.
Seems reasonable yes. Thanks for working on this! |
Use TBB_INC & TBB_LIB environment variables and update the compiler flags accordingly. To use system TBB, both TBB_INC & TBB_LIB environment variables should be defined and the directories of TBB headers and libraries exist. Signed-off-by: Hamada S. Badr <hamada.s.badr@gmail.com>
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thank you and congrats on your first merged PR to the Stan Math repository! 🎉
ifeq ($(OS),Windows_NT) | ||
TBB_TARGETS ?= $(addprefix $(TBB_BIN)/,$(addsuffix $(LIBRARY_SUFFIX),$(TBB_LIBRARIES))) | ||
endif | ||
ifeq ($(OS),Darwin) | ||
TBB_TARGETS ?= $(addprefix $(TBB_BIN)/lib,$(addsuffix $(LIBRARY_SUFFIX), $(TBB_LIBRARIES))) | ||
endif | ||
ifeq ($(OS),Linux) | ||
# Update the suffix with the internal TBB source code! | ||
# The new version of TBB library is 12+ (e.g., libtbb.so.12.1 not libtbb.so.2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads up!
See RcppCore/RcppParallel#141, stan-dev/math/pull/2257, and stan-dev/math/pull/2261. Signed-off-by: Hamada S. Badr <hamada.s.badr@gmail.com>
Summary
This adds support for the new interface of Intel TBB and allows using external library (e.g., with Intel OneAPI), using
TBB_LIB
andTBB_INC
environment variables. To enable the new TBB interface, defineTBB_INTERFACE_NEW
in the user-defined variables in~/.config/stan/make.local
ormake/local
. The updated TBB functionality are summarized here and the release notes and backward compatibility are reported here.Note that the internal TBB source code is ignored if
TBB_LIB
andTBB_INC
environment variables are defined.TBB_INTERFACE_NEW
can be set totrue
by default when the source code is updated. I'm not sure if you'd like to keep backward compatibility with the old TBB interface. If not, this new interface branching can be removed and fall back to the updated source code if the system headers are old (e.g.,tbb/tbb_stddef.h
exists).TBB_INTERFACE_VERSION
is now moved to a new headertbb/version.h
fromtbb/tbb_stddef.h
, which has been removed.PS: Do not enable the new TBB interface with the current internal version of TBB, until it gets updated. Define
TBB_INTERFACE_NEW
,TBB_LIB
andTBB_INC
to use an updated external TBB library.This is related to RcppCore/RcppParallel#141.
Tests
intel_tbb_new_init
intel_tbb_new_late_init
Side Effects
N/A
Release notes
Added support for the new TBB interface and allowed using an external TBB library.
Checklist
Math issue N/A
Copyright holder: Hamada S. Badr hamada.s.badr@gmail.com
the basic tests are passing
./runTests.py test/unit
)make test-headers
)make test-math-dependencies
)make doxygen
)make cpplint
)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested