ARROW-8440: [C++] Refine SIMD header files by cyb70289 · Pull Request #6954 · apache/arrow

cyb70289 · 2020-04-16T03:05:45Z

Include all necessary SIMD header files in a single header "simd.h".
Simplify architecture dependent CRC code in hash_util.h.
Remove SSEUtil namespace which contains some SSE constants.
These codes are not used and I can't find proper place to hold them.
Remove sse_util.h, neon_util.h, and hash_util.h.

cyb70289 · 2020-04-16T03:06:44Z

cc @jianxind

github-actions · 2020-04-16T03:16:32Z

https://issues.apache.org/jira/browse/ARROW-8440

frankdjx

I like the ideal to has one unified SIMD head file for all ARCH. Cool.

frankdjx · 2020-04-20T01:55:35Z

cpp/cmake_modules/DefineOptions.cmake

I personally prefer to keep the NONE(zero) level here though it may duplicate to ARROW_USE_SIMD. Level usually start from zero.

Thanks for review. NONE restored.

wesm · 2020-04-20T12:53:39Z

I thought we had discussed removing the ARROW_USE_SIMD option altogether

cyb70289 · 2020-04-20T14:20:34Z

I thought we had discussed removing the ARROW_USE_SIMD option altogether

@wesm remove ARROW_USE_SIMD? I remember a thread about default simd level( #6907 (comment)), but not ARROW_USE_SIMD.
That said, I would like to remove it as it overlaps other setting.

wesm · 2020-04-20T14:51:45Z

Maybe it was just a thought I had in my head but never expressed. Opened https://issues.apache.org/jira/browse/ARROW-8531

cyb70289 · 2020-04-22T02:49:55Z

Maybe it was just a thought I had in my head but never expressed. Opened https://issues.apache.org/jira/browse/ARROW-8531

Updated this patch to remove ARROW_USE_SIMD

frankdjx · 2020-04-22T06:37:57Z

docs/source/developers/benchmarks.rst

I guess it should be SSE4_2 as it's the default option. ARROW_USE_SIMD is on as default also. Or just rm the cmake-extras?

The context is to show how to pass extra cmake flags. Default value looks not very useful. Maybe change to AVX2?

Get it, then ARROW_SIMD_LEVEL=NONE is fine:)

kiszk · 2020-04-22T14:26:14Z

cpp/src/arrow/util/hash_util.h

How about moving this part into simd.h? We can reduce the number of places that depends on the target hardware.

Also, regardless of whether we move them or not, you should put them in the arrow::internal namespace.

I'd be concerned about putting too much in SIMD directly. If we expect these to be used in other places that don't depend on this header we should create a specific header targetted at hardware. digests/hashing. I think the way bit_util.h wraps special instructions is a reasonable path to follow.

kiszk · 2020-04-22T15:36:33Z

Is the function Armv8CrcHashParallel used somewhere? Sorry if I overlook it.

pitrou

Thank you for doing this. This looks like a good idea.

pitrou · 2020-04-22T15:38:47Z

cpp/src/arrow/util/hash_util.h

Also, regardless of whether we move them or not, you should put them in the arrow::internal namespace.

pitrou · 2020-04-23T10:47:14Z

cc @emkornfield

cyb70289 · 2020-04-24T03:34:27Z

Is the function Armv8CrcHashParallel used somewhere? Sorry if I overlook it.

It's not used. Actually the whole file hash_util.h is not used per this comment. From git
history, I see crc based hash is replaced with xxhash3.

I'm glad to remove this file if community think it's ok.

emkornfield · 2020-04-26T06:04:19Z

cpp/src/arrow/util/hash_util.h

style nits: If possible les spell out the type instead of using auto (its not all clear what the types are without looking up these exact functions.

Since these are functions, it would be nice to use standard function casing for them. e.g. Crc32U16, even better might be to define something like:
HardwareCrc(uint16) ..
HardwareCrc(uint16) ..

emkornfield · 2020-04-26T06:15:55Z

cpp/src/arrow/util/simd.h

Just an FYI in #6985 relies on BMI2 do you think adding that here would be appropriate?

yes, I think so

emkornfield · 2020-04-26T23:08:05Z

Is the function Armv8CrcHashParallel used somewhere? Sorry if I overlook it.

It's not used. Actually the whole file hash_util.h is not used per this comment. From git
history, I see crc based hash is replaced with xxhash3.

I'm glad to remove this file if community think it's ok.

Sorry I missed this, I think we can probably remove these for now (we can always reinstate from git history if needed). Parquet CRC uses standard CRC32 and at least for intel, the intrinsic does CRC32C. This might be useful for digests for Arrow File/Stream but that hasn't been approved by the community yet.

- Include all necessary SIMD header files in a single header "simd.h". - Simplify architecture dependent CRC code in hash_util.h. - Remove SSEUtil namespace which contains some SSE constants. These codes are not used and I can't find proper place to hold them. - Remove sse_util.h and neon_util.h. - Remove ARROW_SIMD_LEVEL=NONE which duplicates ARROW_USE_SIMD=OFF.

cyb70289 · 2020-04-27T02:12:24Z

Sorry I missed this, I think we can probably remove these for now (we can always reinstate from git history if needed). Parquet CRC uses standard CRC32 and at least for intel, the intrinsic does CRC32C. This might be useful for digests for Arrow File/Stream but that hasn't been approved by the community yet.

@kiszk @emkornfield @pitrou , removed hash_util.h, easiest path for me :) , we can pick it back and refine per actual use case if it will be used in future.

pitrou

+1, LGTM. @emkornfield, do you have further comments on this PR? Otherwise I think we can merge.

emkornfield · 2020-05-05T04:03:58Z

Nothing more from me

wesm force-pushed the master branch from 5fe5b88 to aa55967 Compare April 19, 2020 22:46

frankdjx reviewed Apr 20, 2020

View reviewed changes

kszucs force-pushed the master branch from 1b71ca7 to 5093b80 Compare April 20, 2020 19:21

frankdjx reviewed Apr 22, 2020

View reviewed changes

kiszk reviewed Apr 22, 2020

View reviewed changes

pitrou reviewed Apr 22, 2020

View reviewed changes

emkornfield reviewed Apr 26, 2020

View reviewed changes

cyb70289 added 4 commits April 27, 2020 01:31

Restore ARROW_SIMD_LEVEL=NONE

11d95d1

Remove ARROW_USE_SIMD

479b992

Remove hash_util.h

f0813fb

pitrou approved these changes May 4, 2020

View reviewed changes

pitrou closed this in 8e1b5b8 May 5, 2020

cyb70289 deleted the simd branch May 5, 2020 10:54

asfimport mentioned this pull request May 5, 2020

[C++] Refine simd header files #24619

Closed

Conversation

cyb70289 commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyb70289 commented Apr 16, 2020

Uh oh!

github-actions bot commented Apr 16, 2020

Uh oh!

frankdjx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Apr 20, 2020

Uh oh!

cyb70289 commented Apr 20, 2020

Uh oh!

wesm commented Apr 20, 2020

Uh oh!

cyb70289 commented Apr 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emkornfield Apr 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Apr 23, 2020

Uh oh!

cyb70289 commented Apr 24, 2020

Uh oh!

emkornfield Apr 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Apr 26, 2020

Uh oh!

cyb70289 commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

emkornfield commented May 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cyb70289 commented Apr 16, 2020 •

edited

Loading

emkornfield Apr 26, 2020 •

edited

Loading

kiszk commented Apr 22, 2020 •

edited

Loading

emkornfield Apr 26, 2020 •

edited

Loading

cyb70289 commented Apr 27, 2020 •

edited

Loading