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.

Sign up for GitHub

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

Jump to bottom

ARROW-3844: [C++] Remove ARROW_USE_SSE and ARROW_SSE3 #3037

Closed
wants to merge 1 commit into from
Closed

ARROW-3844: [C++] Remove ARROW_USE_SSE and ARROW_SSE3 #3037

wants to merge 1 commit into from

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Nov 26, 2018

Those options can be detected programmatically (SSE3 and SSE4.2 are available on all recent x86-64 CPUs).
Instead we add a ARROW_USE_SIMD option that can be disabled to exercise non-SIMD fallback paths.

@pitrou
Copy link
Member Author

pitrou commented Nov 26, 2018

Note it is debatable whether some functions (such as CRC32 accelerations) are actually "SIMD". Someone may suggest another name if desired.

@pitrou
Copy link
Member Author

pitrou commented Nov 26, 2018

Ah, there is a crash that is due to CrcHash not accepting a null pointer. This shows that the SSE paths in Parquet were entirely not tested.

@pitrou
Copy link
Member Author

pitrou commented Nov 26, 2018

Rather than try to workaround the null pointer issue, I will wait for PR #3036 to be merged and then rebase this PR, since that will change the uses of hash functions by Parquet.

@fsaintjacques
Copy link
Contributor

I think that SIMD is too broad term, e.g. the Altivec options just right underneath might be confused with this new naming.

@pitrou
Copy link
Member Author

pitrou commented Nov 26, 2018

Well, the aim is really to provide a knob to disable all explicit use of optional instructions / intrinsics etc. in the source code. Currently -maltivec seems only used for autovectorization (there is no Altivec-dependent switch in the source).

@pitrou pitrou force-pushed the ARROW-3844-remove-sse-switches branch from bf6c32c to 41ea485 Compare November 28, 2018 19:37
@pitrou
Copy link
Member Author

pitrou commented Nov 28, 2018

Rebased now.

wesm
wesm approved these changes Nov 29, 2018
View reviewed changes
Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

+1

@wesm
Copy link
Member

wesm commented Nov 29, 2018

Rebasing again. Not sure what's broken about double-conversion in conda-forge today

@pitrou @wesm
ARROW-3844: [C++] Remove ARROW_USE_SSE and ARROW_SSE3
8adb98b 
Those options can be detected programmatically (SSE3 and SSE4.2 are
available on all recent x86-64 CPUs).  Instead we add a ARROW_USE_SIMD option
that can be disabled to exercise non-SIMD fallback paths.
@wesm wesm force-pushed the ARROW-3844-remove-sse-switches branch from 41ea485 to 8adb98b Compare November 29, 2018 02:47
@codecov-io
Copy link

Codecov Report

Merging #3037 into master will increase coverage by 1.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #3037     +/-   ##
=========================================
+ Coverage   86.95%   88.06%   +1.1%     
=========================================
  Files         483      425     -58     
  Lines       68606    64888   -3718     
=========================================
- Hits        59658    57144   -2514     
+ Misses       8851     7744   -1107     
+ Partials       97        0     -97
Impacted Files Coverage Δ
cpp/src/arrow/util/cpu-info.cc 75.32% <ø> (ø) ⬆️
cpp/src/arrow/util/bit-util.h 98.81% <ø> (ø) ⬆️
cpp/src/arrow/util/sse-util.h 100% <ø> (ø) ⬆️
cpp/src/plasma/thirdparty/ae/ae.c 72.03% <0%> (-0.95%) ⬇️
go/arrow/array/table.go
go/arrow/math/uint64_amd64.go
go/arrow/internal/testing/tools/bool.go
go/arrow/internal/bitutil/bitutil.go
go/arrow/memory/memory_avx2_amd64.go
go/arrow/array/null.go
... and 53 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd896c9...8adb98b. Read the comment docs.

@wesm wesm closed this in 7800684 Nov 29, 2018
@pitrou pitrou deleted the ARROW-3844-remove-sse-switches branch November 29, 2018 12:53
@guyuqi guyuqi mentioned this pull request Nov 29, 2018
Closed
@asfimport asfimport mentioned this pull request Nov 29, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wesm wesm wesm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pitrou @fsaintjacques @wesm @codecov-io