-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-44098: [C++] Add home made _mm256_set_m128i for compilers who are missing it #44116
Conversation
|
@github-actions crossbow submit test-r-rstudio-r-base-4.1-opensuse155 |
Revision: 9134050 Submitted crossbow builds: ursacomputing/crossbow @ actions-0fe409cd3d
|
@@ -27,18 +27,25 @@ | |||
#else | |||
// gcc/clang (possibly others) | |||
|
|||
# if defined(ARROW_HAVE_BMI2) | |||
# if defined(ARROW_HAVE_BMI2) || defined(ARROW_HAVE_RUNTIME_BMI2) |
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.
I try to find some common place to put simd specific code and found this one. However it seems this file is only adapting the static simd optimizations, thus the additions of the ARROW_HAVE_RUNTIME_*
which are for dynamic simd optimizations.
W/o these || defined(ARROW_HAVE_RUNTIME_*
, one won't see <immintrin.h>
inclusion because there are no ARROW_HAVE_*
macros defined. (This might be yet another issue of our current simd macro organization).
Also cc @pitrou . |
|
@ursabot please benchmark lang=C++ |
Benchmark runs are scheduled for commit 9134050. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete. |
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 9134050. There were 15 benchmark results indicating a performance regression:
The full Conbench report has more details. |
The so-called performance regressions are just flukes. |
@github-actions crossbow submit -g cpp -g r -g wheel |
Revision: 9134050 Submitted crossbow builds: ursacomputing/crossbow @ actions-8d654932f7 |
Nice catch? Can we also try to try this to a gcc 7.5 and godbolt? |
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.
+1, thank you @zanmato1984
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 87d6477. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
AVX2 intrinsic _mm256_set_m128i is missing in GCC versions <8.0 - this is a GCC bug discussed in [1], causing certain CI build failed.
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80582
What changes are included in this PR?
Check the GCC version and use a home made replacement if necessary.
Are these changes tested?
Manually tested.
Are there any user-facing changes?
None.
_mm256_set_m128i()
is unavailable on OpenSUSE 15.1 (GCC 7.5) #44098