Skip to content

Conversation

jgiannuzzi
Copy link

@jgiannuzzi jgiannuzzi commented Oct 10, 2025

Rationale for this change

This change enables building Arrow C++ for Windows ARM64 with MSVC when setting ARROW_SIMD_LEVEL to NONE. This is useful to be able to build Arrow with vcpkg and use it as a dependency.

Setting ARROW_SIMD_LEVEL to NONE is done to disable the use of xsimd, which does not yet support msvc arm64 intrinsics, and is non-trivial to fix.

What changes are included in this PR?

A patch to the vendored pcg library, based on imneme/pcg-cpp#99. The upstream pcg library has not been updated in the past 3 years, so this may never get merged.

Are these changes tested?

Yes, the changes have been tested in microsoft/vcpkg#47750 (the same patch for vcpkg, which this change would alleviate) and in https://github.com/jgiannuzzi/ParquetSharp/actions/runs/18354286294 (a full run of the ParquetSharp CI, using this patch to build Arrow with vcpkg).

Are there any user-facing changes?

Not really, unless you consider adding the ability to build Arrow on Windows ARM64 user-facing?

This enables building Arrow C++ for Windows ARM64 with MSVC when setting
ARROW_SIMD_LEVEL to NONE (to disable xsimd usage, which does not support
msvc arm64 intrinsics, and is non-trivial to fix).

This patch is based on imneme/pcg-cpp#99.
The upstream pcg library has not been updated in the past 3 years, so
this may never get merged.
Copy link

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@jgiannuzzi jgiannuzzi changed the title [C++] Patch vendored pcg library to enable msvc arm64 intrinsics GH-47784: [C++] Patch vendored pcg library to enable msvc arm64 intrinsics Oct 10, 2025
Copy link

⚠️ GitHub issue #47784 has been automatically assigned in GitHub to PR creator.

@jgiannuzzi jgiannuzzi marked this pull request as ready for review October 10, 2025 17:35
@kou
Copy link
Member

kou commented Oct 11, 2025

Hmm. It seems that https://github.com/imneme/pcg-cpp isn't maintained...

Do we have alternative implementation...?

https://github.com/imneme/pcg-cpp was vendored by #8879. @pitrou Do you have any opinion?

@pitrou
Copy link
Member

pitrou commented Oct 11, 2025

Setting ARROW_SIMD_LEVEL to NONE is done to disable the use of xsimd, which does not yet support msvc arm64 intrinsics, and is non-trivial to fix.

@JohanMabille @AntoinePrv @serge-sans-paille Do you think xsimd can support ARM64 intrinsics on MSVC?

@pitrou
Copy link
Member

pitrou commented Oct 11, 2025

A patch to the vendored pcg library, based on imneme/pcg-cpp#99. The upstream pcg library has not been updated in the past 3 years, so this may never get merged.

What does this patch have to do with xsimd? Am I missing something?

@jgiannuzzi
Copy link
Author

jgiannuzzi commented Oct 11, 2025

What does this patch have to do with xsimd? Am I missing something?

Sorry @pitrou, my description wasn't clear. This patch has nothing to do with xsimd. I just meant to say that this pcg patch is not enough to build arrow with msvc on arm64 - you also need to set ARROW_SIMD_LEVEL to NONE to switch off ARROW_HAVE_NEON, which in turn will disable the use of xsimd, as it doesn't build with msvc for arm64.

@jgiannuzzi
Copy link
Author

@JohanMabille @AntoinePrv @serge-sans-paille Do you think xsimd can support ARM64 intrinsics on MSVC?

I found this PR created in 2021 attempting to solve this: xtensor-stack/xsimd#612.

@pitrou
Copy link
Member

pitrou commented Oct 12, 2025

Hmm. It seems that https://github.com/imneme/pcg-cpp isn't maintained...

Do we have alternative implementation...?

There is a fork here by @brt-v: https://github.com/brt-v/pcg-cpp?tab=readme-ov-file#about-this-fork
I'm not sure how reasonable it is to rely on it, though.

@brt-v
Copy link

brt-v commented Oct 12, 2025

Hmm. It seems that https://github.com/imneme/pcg-cpp isn't maintained...
Do we have alternative implementation...?

There is a fork here by @brt-v: https://github.com/brt-v/pcg-cpp?tab=readme-ov-file#about-this-fork I'm not sure how reasonable it is to rely on it, though.

I made that fork because of the lack of maintenance of the original implementation, with mostly quality of life improvements. The logic and tests are identical to the original, excepts for some small fixes to get all tests working on Windows. Note that I'm not an rng researcher and have no intention in further developing the algorithm, I just wanted to provide an easy to use library. I understand to you may not want to rely on such a small/unknown repo, but I'm happy to address your concerns and make further improvements where needed.

@JohanMabille
Copy link
Contributor

JohanMabille commented Oct 13, 2025

Setting ARROW_SIMD_LEVEL to NONE is done to disable the use of xsimd, which does not yet support msvc arm64 intrinsics, and is non-trivial to fix.

@JohanMabille @AntoinePrv @serge-sans-paille Do you think xsimd can support ARM64 intrinsics on MSVC?

Yes it could, but I cannot give a deadline for that. For the record, an issue for tracking this feature request has already been opened.

@pitrou
Copy link
Member

pitrou commented Oct 13, 2025

I understand to you may not want to rely on such a small/unknown repo, but I'm happy to address your concerns and make further improvements where needed.

Well, I think we could at some point. For now, we still support C++11 so we'll have to patch our vendored copy instead.

@kou
Copy link
Member

kou commented Oct 13, 2025

we still support C++11

Really? I think that we require C++17:

# This ensures that things like c++17 get passed correctly
if(NOT DEFINED CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 17)
elseif(${CMAKE_CXX_STANDARD} VERSION_LESS 17)
message(FATAL_ERROR "Cannot set a CMAKE_CXX_STANDARD smaller than 17")
endif()

@pitrou
Copy link
Member

pitrou commented Oct 13, 2025

Really? I think that we require C++17:

Wow. I even did the change myself. Clearly I haven't drunk enough tea this morning...

(I was thinking of C++20 for some reason)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants