Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ORC-1356: [C++] Use Intel AVX-512 instructions to accelerate the Rle-bit-packing decode #1375
ORC-1356: [C++] Use Intel AVX-512 instructions to accelerate the Rle-bit-packing decode #1375
Changes from 15 commits
58c3ab6
acbc214
293d863
e7a9119
cfde08f
8341943
e840649
c7962d5
495a620
5c937e6
a87c281
d8fcbe6
668335c
46daa2d
415d1eb
cd2f71d
f9ee0b4
6f8cb56
c1c2448
edf164f
f360582
743ac84
6bc9035
ca3af78
eeafccf
1beb9b5
d9c562b
0cf5620
08e32f4
8a6b9f7
1b8301f
1924ecf
3c4f2b8
b1759a1
dc81e79
b37c7dd
23dd7ff
6a6f491
b2abf44
36f06aa
3db8d1a
15db3d1
d77c81b
42cc703
4fbe1d7
9d86e3d
75e4cfa
284a9a4
2c9f93f
d21705c
197f2e6
1d050af
10b7009
a239e47
b2b6aff
6f06b79
a0aa823
d7112e9
5b38980
2ad64bc
6768165
d383035
ce7f6de
8f6806b
e27be9e
fe5b6c7
5b0e66d
8c99fcd
0f1adda
440d6d1
070ca0f
21de59a
ae0d5c2
3f47d1c
1fdfe54
3f156b4
4b166ee
3c21f2e
27d5b40
7cea68e
3be42ee
11ceeaa
277d9be
305a317
4debd50
62d373c
3dca1d7
e23ca29
1a32212
fc2c288
3468df0
93feaf9
3b831f2
1deb2cf
b48ec06
b89870a
fe09a92
596835d
e236773
321ab63
df6fe45
ce77b50
6c84d8d
f3ff215
af96de9
d6fd57d
0bfc862
e584a42
4d261eb
1f2085e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
These options and variables look confusing to me.
BUILD_ENABLE_AVX512
andORC_SIMD_LEVEL
serve the same purpose. At least one of them should be removed.If
ORC_SIMD_LEVEL
andORC_RUNTIME_SIMD_LEVEL
only have default values, then they should be removed because they cannot be changed. Otherwise, they should at least supportNONE
andAVX512
to be configurable.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.
Fixed. Delete ORC_RUNTIME_SIMD_LEVEL
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.
The architecture detecting logic below worth a separate file under
cmake_modules
directory and be included here.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.
Please move lines from 175 to 270 into a separate cmake module.
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.
Are we supposed to support ppc, s390x and riscv64? The CI checks do not cover these architectures so we are unable to verify and maintain them.
cc @dongjoon-hyun
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.
This fails the build on these processors, though it rarely happens. At least we should not break build which succeeds in the past.
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.
Created a new cmake module "cmake_modules/ConfigSimdLevel.cmake" to config AVX512.
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.
If this patch aims for AVX512 only, we can remove SSE4 and AVX2 for now. So flags like
ORC_AVX2_FLAG
,CXX_SUPPORTS_SSE4_2
, andCXX_SUPPORTS_AVX2
can be removed for now.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.
CXX_SUPPORTS_SSE4_2
is not used and can be removed.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.
Fixed
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.
Why not use a single set for
ORC_AVX512_FLAG
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.
Fixed.
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.
What does
fix issue ORC-9877 for homebrew-cpp
mean?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.
Sorry for bad reference, already deleted.
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.
It seems that
CMAKE_REQUIRED_FLAGS
is not officially documented. Do we any have better alternatives?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.
we can find the CMAKE_REQUIRED_FLAGS information in the cmake document:
https://cmake.org/cmake/help/latest/module/CheckCXXSourceCompiles.html
Is there no need to change CMAKE_REQUIRED_FLAGS ? Is my understanding right?
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.
Please remove the logic relevant to
aarch64
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.
Fixed
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.
We can remove levels other than
AVX512
for now to make it simpler.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.
Platform dependent function like
cpuid
can be defined in the fileAdaptor.hh.in
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.
Should we rename the function and the file name to
detect architecture
?