Skip to content

Conversation

@wesm
Copy link
Member

@wesm wesm commented Apr 13, 2020

These platforms don't support -march=armv8-a+crc+crypto so don't think this makes sense to have as a default.

Since ORC has released since a build flag was added to turn off -Werror I've upgraded it here and removed the previous warning workarounds

This also resolves ARROW-7968

@github-actions
Copy link

define_option_string(ARROW_ARMV8_ARCH "Arm64 arch and extensions" "armv8-a+crc+crypto")
define_option_string(ARROW_ARMV8_ARCH
"Arm64 arch and extensions"
"" # default to None
Copy link
Contributor

Choose a reason for hiding this comment

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

We can change the default value to "armv8-a"

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll check this out on my rockpro64 and then make the changes below if all looks good

set(ARROW_ALTIVEC_FLAG "-maltivec")
check_cxx_compiler_flag(${ARROW_ALTIVEC_FLAG} CXX_SUPPORTS_ALTIVEC)
elseif(ARROW_CPU_FLAG STREQUAL "arm")
elseif(ARROW_CPU_FLAG STREQUAL "arm" AND NOT ("${ARROW_ARMV8_ARCH}" STREQUAL ""))
Copy link
Contributor

Choose a reason for hiding this comment

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

then this change is not necessary

endif()

if(ARROW_CPU_FLAG STREQUAL "arm")
if(ARROW_CPU_FLAG STREQUAL "arm" AND NOT ("${ARROW_ARMV8_ARCH}" STREQUAL ""))
Copy link
Contributor

Choose a reason for hiding this comment

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

and this change not necessary too

@cyb70289
Copy link
Contributor

cyb70289 commented Apr 13, 2020

These platforms don't support -march=armv8-a+crc+crypto so don't think this makes sense to have as a default.

Maybe pass -DARROW_ARMV8_ARCH=armv8-a when building these platforms and leave the default as is?
True Arm64 servers(I mean servers deployed seriously in data center) do support these extensions.

@wesm
Copy link
Member Author

wesm commented Apr 13, 2020

Maybe pass -DARROW_ARMV8_ARCH=armv8-a when building these platforms and leave the default as is?

Having the default build fail on some ARMv8 platforms (on "unserious" ones, I guess =) ) doesn't seem right to me. I would push back against having -march=avx2 as the default for x64 builds for the same reason. These options are primarily intended for people building packages or developers optimizing for certain CPU capabilities

@wesm
Copy link
Member Author

wesm commented Apr 13, 2020

@pitrou thoughts?

@kszucs
Copy link
Member

kszucs commented Apr 13, 2020

@wesm can you test this with docker?

ARCH=arm64v8 docker-compose build ubuntu-cpp
ARCH=arm64v8 docker-compose run --rm ubuntu-cpp

probably need to add to the volumes section of docker-compose.yml

  arm64v8-conda-cache:
  arm64v8-cuda-9.1-cache:
  arm64v8-cuda-10.0-cache:
  arm64v8-cuda-10.1-cache:
  arm64v8-debian-9-cache:
  arm64v8-debian-10-cache:
  arm64v8-fedora-30-cache:
  arm64v8-ubuntu-14.04-cache:
  arm64v8-ubuntu-16.04-cache:
  arm64v8-ubuntu-18.04-cache:
  arm64v8-ubuntu-20.04-cache:

Copy link
Member

@pitrou pitrou 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 Author

wesm commented Apr 13, 2020

@kszucs I'd rather not fiddle more with this, we can tackle Dockerizing the ARM tests in a subsequent PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants