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.

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

ARROW-5196 [C++] Uniform usage of Google cpu_features library accross the codebase #4201

Closed
wants to merge 8 commits into from

Conversation

@aregm
Copy link
Contributor

aregm commented Apr 25, 2019

Resolves the ARROW-5196, Draft pull request to check on various platforms.

@wesm

This comment has been minimized.

Copy link
Member

wesm commented Apr 25, 2019

Hm, I erroneously got the idea that the cpu_features library was more minimalistic than it actually is, I apologize for proposing to vendor it. @pitrou do you think we should add this to conda-forge and the thirdparty toolchain?

@pitrou

This comment has been minimized.

Copy link
Contributor

pitrou commented Apr 25, 2019

Is there a more lightweight alternative to this library? I'm not even sure we're using any of this currently, do we?

@wesm

This comment has been minimized.

Copy link
Member

wesm commented Apr 25, 2019

At one point we were querying supported SIMD instructions; I think it's important to determine at least

  • CPU cache sizes (since algorithms can behave much differently when calibrated based on these)
  • whether AVX2 and AVX512 are available
@pitrou

This comment has been minimized.

Copy link
Contributor

pitrou commented Apr 25, 2019

Ok, it seems that cpu_features is our most reliable option.

We may want to make a conda-forge package ultimately. Though first we can simply add it to our thirdparty libs and build it on the fly (it's actually very fast to build by default).

@pitrou

This comment has been minimized.

Copy link
Contributor

pitrou commented Apr 25, 2019

Side note: cpu_features also provides an executable which can output CPU info in JSON format @fsaintjacques :

$ list_cpu_features 
arch            : x86
brand           : AMD Ryzen 7 1700 Eight-Core Processor          
family          :  23 (0x17)
model           :   1 (0x01)
stepping        :   1 (0x01)
uarch           : AMD_ZEN
flags           : aes,avx,avx2,bmi1,bmi2,cx16,f16c,fma3,movbe,popcnt,rdrnd,sha,sse4_1,sse4_2,ssse3
$ list_cpu_features --json
{"arch":"x86","brand":"AMD Ryzen 7 1700 Eight-Core Processor          ","family":23,"model":1,"stepping":1,"uarch":"AMD_ZEN","flags":["aes","avx","avx2","bmi1","bmi2","cx16","f16c","fma3","movbe","popcnt","rdrnd","sha","sse4_1","sse4_2","ssse3"]}
@aregm

This comment has been minimized.

Copy link
Contributor Author

aregm commented Apr 25, 2019

@pitrou I'll create conda package and add to conda-forge, and redo the PR accordingly. BTW AppVeyor is failing with an error: "CMake Error at C:/Program Files (x86)/CMake/share/cmake-3.13/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
Could NOT find DoubleConversion (missing: DoubleConversion_LIB
DoubleConversion_INCLUDE_DIR)"
But I cannot reproduce i neither in our build system, nor in local build. Have you encountered this before?

@pitrou

This comment has been minimized.

Copy link
Contributor

pitrou commented Apr 25, 2019

@aregm You shouldn't lose your breath over AppVeyor for now ;-) The priority is to revamp the PR using the discussed approach (i.e. build cpu_features using our usual "thirdparty toolchain" build chain, rather than vendor it).

@aregm

This comment has been minimized.

Copy link
Contributor Author

aregm commented Apr 25, 2019

@pitrou, I am just curious what is the cause, but do not plan to spend time on it :)
I plan to create conda package and not another folder thirdparty. I think it is more clean solution. Python guys will help me to publish it so, everybody will benefit.

@pitrou

This comment has been minimized.

Copy link
Contributor

pitrou commented Apr 25, 2019

We still need to allow building it from cmake_modules/ThirdpartyToolchain.cmake. conda is not a requirement for building Arrow.

@aregm

This comment has been minimized.

Copy link
Contributor Author

aregm commented Apr 30, 2019

@pitrou @wesm I have added cpu_features to the conda-forge, but as it was suggested to add to thirdparty - do you have any fast "best-known method" or doc how to add external dependency to the Thirdparty with all the targets and defines correctly propagated through cmakefiles - I tried to replicate double-conversion way, but still includes and libs do not found, and I do not want to spend time on deciphering all the nested cmake calls in --trace if there is a faster way and I do not have to. Thanks!

@pitrou

This comment has been minimized.

Copy link
Contributor

pitrou commented Apr 30, 2019

Unfortunately each external dependency requires its own treatment (the sad fact is that there's no standard build system or procedure in C++), so there's no "easy" way to do it. Usually I would first try to build the dependency by hand, observe the exact options required and where the various build products are installed (include files, library files), and start from there to try replicating it from CMake.

Of course, you can still steal some inspiration for the CMake boilerplate from the existing thirdparty declarations.

More generally you need to:

  • add new entries in thirdparty/versions.txt
  • add a new entry in ARROW_THIRDPARTY_DEPENDENCIES
  • add a new entry in the build_dependency macro
  • add a URL definition snippet (for downloading) as for other thirdparty libs
  • define the build_XXX macro for your XXX dependency
  • add some code that detects the third-party library and/or calls into build_dependency
@emkornfield

This comment has been minimized.

Copy link
Contributor

emkornfield commented Jun 12, 2019

@aregm what is the status of this PR?

@aregm

This comment has been minimized.

Copy link
Contributor Author

aregm commented Jun 14, 2019

@emkornfield The status of feature is pending - I need to add cache info parsing to cpu_features as it occurs they are not parsed there and then pull it in. If you are ok, to go without cache info for a while, until the next release, then I can do it as is.

@kszucs kszucs force-pushed the apache:master branch 2 times, most recently from ed180da to 85fe336 Jul 22, 2019
@aregm aregm closed this Aug 9, 2019
@aregm aregm force-pushed the aregm:ARROW-5196 branch from 565789e to 062cc70 Aug 9, 2019
@aregm aregm reopened this Aug 21, 2019
@aregm aregm marked this pull request as ready for review Aug 21, 2019
@bkietz

This comment has been minimized.

Copy link
Contributor

bkietz commented Aug 22, 2019

@aregm looks like you have merge conflicts, could you please rebase this on master?

@emkornfield

This comment has been minimized.

Copy link
Contributor

emkornfield commented Oct 16, 2019

@aregm I'm going to close this PR for now, please reopen if you have time to rebase, and address any additional feedback. If I understand correctly I think this got blocked on appropriate integration with the build system?

@aregm

This comment has been minimized.

Copy link
Contributor Author

aregm commented Oct 16, 2019

@emkornfield Integration is done in PR, but is old and needs to be rebased on top of the latest master. Need to do that as soon as I get some free cycles. If you need this, I'll do this week.

@emkornfield

This comment has been minimized.

Copy link
Contributor

emkornfield commented Oct 16, 2019

@aregm, I don't think its urgent, I'm just trying to clear out PRs that appear to have stagnated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.