BLD: Add xsimd Dependency and SIMD detection#65471
Conversation
83e8ccf to
2975107
Compare
There was a problem hiding this comment.
I still think we should just remove this new CI job and setup argument; I understand what you are trying to do by covering platforms that we might not be testing, but on the flip side I'd just stick with our current support model of "we only support platforms that we have in CI."
It makes the communication model much easier; otherwise this CI job / setup argument have the tendency to hang around forever and we spend time supporting very niche (and also vague) use cases
There was a problem hiding this comment.
Removed the job, but I think that the option is still beneficial to run the tests (at least locally) for the scalar version.
| 'sse2': is_msvc_syntax ? ['/arch:SSE2'] : ['-msse2'], | ||
| 'avx2': is_msvc_syntax ? ['/arch:AVX2'] : ['-mavx2'], | ||
| 'avx512cd': is_msvc_syntax ? ['/arch:AVX512'] : ['-mavx512cd'], | ||
| 'neon': [], |
There was a problem hiding this comment.
If this doesn't have any flags why include here?
| supported_simd_archs += {name: flags} | ||
| endif | ||
| endif | ||
| elif host_machine.cpu_family() == 'aarch64' |
There was a problem hiding this comment.
Related to other comment, I think it makes more sense to move this out of the loop
| endif | ||
|
|
||
| foreach arch_name, arch_flags : supported_simd_archs | ||
| simd_config.set('PANDAS_HAVE_@0@'.format(arch_name.to_upper()), 1) |
There was a problem hiding this comment.
I think its easier to just set this in the loop above rather than its own dedicated one
eefe359 to
0fefde7
Compare
WillAyd
left a comment
There was a problem hiding this comment.
minor comment but otherwise lgtm. anyone else care to look? maybe @mroeschke @jorisvandenbossche @jbrockmendel
| @@ -0,0 +1,5 @@ | |||
| option( | |||
There was a problem hiding this comment.
Can we get rid of this option? Or is someone asking for this?
Since its opt-out it seems really unlikely to be used all that much; if someone wants to champion it in the future let's leave it to them to do so separately
How does the feature toggle work exactly? Can you give some examples? |
Oh, sorry, this is outdated. @WillAyd requested to remove it. The CI job was removed in ed588ad. The meson option to disable SIMD was removed in effdd43. Before, it was a meson option that allowed to disable SIMD by passing the option |
|
Ah, I see there is some relevant content in the inline review discussions with Will
I think we ourselves already need such an option? I assume this PR enables to build with AVX extensions, but we do not want to use those for our wheel builds. So for that we would already need to be able to disable AVX. Also, to avoid creating incompatible binaries, the default should maybe not entirely be opt-out. It could also default to some baseline (#64884 (comment))
As long as we have scalar code included in pandas, I think we should ensure test coverage for that code (so for me it is more about test coverage for the code (in case someone builds pandas from source with some compiler flag that disables optimizations) than "platform support"; we still support only the platforms in CI, but there are various options how to build pandas on those platforms) In any case, this also touches more upon the general questions of what is needed for us to start adding simd code, so very welcome to chime in on #64884 |
This might not actually true what I said above (the "detection" is based on existing compiler arguments, not actually detecting what is supported on the machine?) But so it might be good to try to clarify what this PR is exactly adding then |
This PR adds boilerplate in the build system for SIMD capability. It adds xsimd as a dependency and checks compiler capability to compile SIMD instructions for arm64 and x86. This PR is also a base for #64905, which uses CPU feature detection from xsimd for runtime dispatch. In there, I compile scalar, SSE2, AVX2 and AVX512 versions of the code. |
Should I remove the AVX2 and AVX512 targets to don't have to deal with runtime dispatch for now? It also raises the question if the 32-bit version of x86 should only have the scalar version, since it's not guaranteed that the CPU supports SSE2 instructions. |
So it defines the I just want us to be very explicit in what is being added here. Because eg #64515 (before the last 2 commits that pushed a refactor with xsimd) was also defining (inline) eg |
|
For example, in Arrow C++ they distinguish between |
Yes. But also probing for the macros
No, it's also useful for compile‑time dispatch. The
I think you meant #64515. The variables defined here serve the same purpose as the inline definition there, they can be used identically. So, instead of using There is also an ongoing discussion with @WillAyd about removing these macros in #64905, but we haven't found a nice replacement yet. |
|
I will modify this PR and #64905 to only target the aarch64 and x86-64 baselines. Probably it's better to leave dispatch for later. |
Co-authored-by: William Ayd <william.ayd@icloud.com>
effdd43 to
772767c
Compare
772767c to
aab9418
Compare
Sure, in both cases the definitions are evaluated at compile time, but my main point I tried to make is that they have a different intended usage pattern:
But then I'll repeat from above (#65471 (comment)): the way this PR was defining those variables would not be useful for actual compile-time dispatch, without a build flag to be able to control it. So if a (and eg Arrow C++ is having two sets of flags for both cases) Now, in the meantime you updated the PR to only cover the baseline, which are exactly those cases where there is no difference between both cases ;)
Whoops yes, updated! |
This PR adds boilerplate for SIMD detection,
feature toggle and a CI job disabling SIMD.Was part of #64905 and added a few modifications:
PANDAS_HAVE_SCALARso that in case our SIMD dispatch logic if flawed won't cause SIGILL.dependencyinstead ofsubprojectfor xsimd, but requires a minimum version ofxsimdof>=14.0to don't have to differentiate betweenxsimd::commonandxsimd::generic; and requires the latestxsimd>=14.2for MSVC + ARM compatibility.It doesn't add any SIMD code, the main visible change is the creation of
pandas_simd_config.hcc @WillAyd