-
Notifications
You must be signed in to change notification settings - Fork 59
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 #141
Conversation
Set TBB_LIBRARY_FILE environemnt variable to use Intel/OneAPI TBB: export TBB_LIBRARY_FILE=$TBBROOT/lib/$TBB_LIB_DIR/libtbb.so Modified from Debian patch by Andreas Tille <tille@debian.org>: https://salsa.debian.org/r-pkg-team/r-cran-rcppparallel/-/blob/master/debian/patches/use_debian_packaged_libtbb.patch
See https://software.intel.com/content/www/us/en/develop/articles/tbb-revamp.html Signed-off-by: Hamada S. Badr <hamada.s.badr@gmail.com>
Conceptually speaking, could you foresee a modification where both a vendored and a system library could be used? I contributed such a scheme once to another (smaller) package---in the R ecosystem we have our reasons for vendoring to ensure "easy" buildability for everybody across multiple operating systems and many different versions. Now, I have a lot of sympathy for the "system" view and yet while I am contributor to one of the Linux distros still "embed" libraries in R package "CRAN style" (and am currently doing such an upgrade for Boost 1.75.0 in the BH package I maintain). This a call to make for @kevinushey but I fear removing the embedded source may create problems for the package. |
Signed-off-by: Hamada S. Badr <hamada.s.badr@gmail.com>
@eddelbuettel Agreed. I'm testing this right now because I have recognized that this fails if the environment is not set correctly (i.e., if TBB library is not found in the system or if the environment variable |
`__TBB_tbb_stddef_H` is defined in `tbb/tbb_stddef.h`, which has been removed in the new TBB interface.
Right, we need to keep the embedded code so this can still build on Windows + macOS. I agree that having RcppParallel use the system TBB library on Linux-alikes would be worth doing (and we should make it possible for the user to configure which version is loaded at runtime regardless). |
This is not ready yet... need to fix the conflicts with system headers and test it. |
@kevinushey This is ready to go. Everything has been summarized in the PR descriptions and the package NEWS. Users can opt in using the system TBB library or building it from the embedded source code at compilation time; header conflicts are automatically resolved. This should be it since runtime changes is tricky since the package shared library uses TBB headers and, with the new TBB interface that breaks backward compatibility, it isn't recommended to change the loaded TBB library without recompilation. |
a0476db
to
3b45f07
Compare
Use TBB_INC & TBB_LIB environment variables and update CXXFLAGS 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>
60cba88
to
aefff2d
Compare
Thanks; I'll try to review this soon. (I see you've been making a couple more commits since you said this was ready for review so just want let the dust settle first) |
@kevinushey Sounds good! It was ready, but then I've been fixing reverse dependencies for packages that link directly to the libs in The conflicting headers actually break the current version of |
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.
Overall this looks good to me -- thank you for putting this together! Some questions I have:
If RcppParallel is built using the system version of TBB, should it "remember" the TBB include / library paths used at build time? Or is it okay to continue respecting TBB_LIB
and TBB_INC
at runtime, even if it differs? (What happens if TBB_LIB + TBB_INC are set at build time, but are not set at runtime -- can the package still run?)
What's the right way for users installing RcppParallel
to use the system copy of TBB? Does it suffice to just set the TBB_INC and TBB_LIB environment variables, or do they have to be passed as configure variables?
-r is declared obsolete by POSIX and can have implementation-defined behavior. Co-authored-by: Kevin Ushey <kevinushey@gmail.com>
Yes. Though, the environment variables are required to report the correct
So, it's recommend to keep the environment variables defined at runtime. They can be updated to use different paths too, if the different TBB versions are compatible (use the same interface). But, we already copy the libs inside
The users need to set the environment variables (specifically:
|
@kevinushey The warnings I mentioned above are from |
This fixes the suffix issue with newer versions of TBB.
Signed-off-by: Hamada S. Badr <hamada.s.badr@gmail.com>
Signed-off-by: Hamada S. Badr <hamada.s.badr@gmail.com>
$(TBB_INC)/serial doesn't exist in the new version of TBB. Signed-off-by: Hamada S. Badr <hamada.s.badr@gmail.com>
@kevinushey With the above commits, the package should run in all cases. |
Awesome; thank you very much! |
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>
This closes #107 and is related to stan-dev/math#2257. It adds support for the new interface of Intel TBB and allows using external library (e.g., with Intel OneAPI), using
TBB_LIB
andTBB_INC
and environment variables. The updated TBB functionality are summarized here and the release notes and backward compatibility are reported here.These changes have been successfully tested with both embedded TBB source code and TBB library from Intel OneAPI 2021.1.
TBB_INC
and/orTBB_LIB
environment variables__TBB_tbb_stddef_H