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

Support building TBB on Windows arm64 #215

Merged
merged 4 commits into from
May 20, 2024

Conversation

andrjohns
Copy link
Contributor

This PR reverts the changes which delegate to RTools for the TBB on Windows ARM64, and adds support for building and using the bundled TBB.

We use the same changes in the Stan Math library, where the threading all performs as expected on Windows ARM64 with the RTools aarch64 toolchain.

@kalibera would you be able to verify on your system as well, when you have the time?

@kalibera
Copy link

Thanks for taking the time to figure out how to build the old bundled version of TBB on aarch64.

I think in the longer term it would be necessary, anyway, to upgrade to a newer one (such as that in Rtools). And it might be a good thing if RcppParallel easily allowed building with the TBB version in Rtools so that maintainers of the reverse dependencies could easily check their code works with both versions: on all platforms, not just Windows/aarch64.

@kalibera would you be able to verify on your system as well, when you have the time?

RcppParallel itself with the patch builds and passes checks on my system. If you haven't done that already, it would be good to check also all reverse dependencies.

@andrjohns
Copy link
Contributor Author

RcppParallel itself with the patch builds and passes checks on my system. If you haven't done that already, it would be good to check also all reverse dependencies.

Great! I'll check the reverse-dependencies over the next few days and report back.

And it might be a good thing if RcppParallel easily allowed building with the TBB version

I believe it already does, with the TBB_INC and TBB_LIB variables.

But I think RcppParallel will always need to have a bundled version of the TBB for Windows, otherwise users of binary packages which dynamically link (recommended by TBB) will also need to have RTools installed

Copy link
Contributor

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

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

Overall looks good to me; only concern is making sure the patch we're making to TBB either already exists upstream, or has some way of preserving the patch action for future upgrades of TBB.

@@ -25,7 +25,7 @@
# define __ARCH_ipf 1
# elif defined(_M_IX86)||defined(__i386__) // the latter for MinGW support
# define __ARCH_x86_32 1
# elif defined(_M_ARM)
# elif defined(_M_ARM) || defined(__aarch64__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if this change exists upstream as well in newer TBB versions? I worry a bit about losing TBB patches in our bundled copy if we later want to pull a newer version of TBB into RcppParallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately Windows ARM64 compatibility was only officially added in oneTBB, so there isn't an upstream reference to draw from.

But to help record the changes, I've copied the approach from RcppEigen and added a patches directory with a diff of the TBB changes

src/tbb/build/Makefile.tbb Outdated Show resolved Hide resolved
src/Makevars.in Outdated Show resolved Hide resolved
@kalibera
Copy link

And it might be a good thing if RcppParallel easily allowed building with the TBB version

I believe it already does, with the TBB_INC and TBB_LIB variables.

I didn't check the trunk version, but that wasn't the case, one had to do more (adjust to different library names, allow this on Windows) - that's what my pull request which I used for testing did, to be able to build with Rtools/MXE build of oneTBB.

https://svn.r-project.org/R-dev-web/trunk/WindowsBuilds/winutf8/ucrt3/r_packages/patches/CRAN/RcppParallel.diff

But I think RcppParallel will always need to have a bundled version of the TBB for Windows, otherwise users of binary packages which dynamically link (recommended by TBB) will also need to have RTools installed

It would be worth checking why they recommend that and whether it is still needed in our context, I'm not aware running into problems with static linking. But, in principle, some R packages already copy some files from Rtools into their installation directories, typically some data/configuration.

@andrjohns
Copy link
Contributor Author

After checking the reverse-dependencies, I found that RcppParallel.h also needed to define the TBB_USE_GCC_BUILTINS flag since the majority of downstream packages don't call RcppParallel::CxxFlags() in their Makevars.

This would still be an issue for any downstream packages which include the TBB headers directly, but thankfully the majority of those inherit flags from StanHeaders - so once I update the StanHeaders::CxxFlags() call that they use, they build without issue.

There were two remaining packages that used TBB headers directly without inheriting flags, but they've since merged PRs to add RcppParallel::CxxFlags() to their Makevars: stan4bart & MultiBD.

So now all downstream dependencies (or their development versions) are fine on Windows ARM64. It would be great to also update the documentation to suggest adding RcppParallel::CxxFlags() to PKG_CXXFLAGS, if possible

Copy link
Contributor

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

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

LGTM; thank you!

@kevinushey kevinushey merged commit 8ace319 into RcppCore:master May 20, 2024
@andrjohns andrjohns deleted the tbb-winarm64 branch May 20, 2024 16:35
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.

None yet

3 participants