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

build/ops: arch: fix cmake's ARM CRC intrinsics test to handle duplicitous gcc 4.8.5 (issue#19386, pr#14132, Dan Mick) #14132

Merged
merged 2 commits into from Mar 27, 2017

Conversation

Projects
None yet
2 participants
@dmick
Member

dmick commented Mar 25, 2017

No description provided.

cmake/modules/SIMDExt.cmake: add whitespace
readability, man, readability

Signed-off-by: Dan Mick <dan.mick@redhat.com>

@dmick dmick changed the title from Fix cmake's ARM CRC intrinsics test to handle duplicitous gcc 4.5.8 to Fix cmake's ARM CRC intrinsics test to handle duplicitous gcc 4.8.5 Mar 25, 2017

@dmick

This comment has been minimized.

Show comment
Hide comment
@dmick

dmick Mar 25, 2017

Member

Actual test build here:

https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=arm64,AVAILABLE_ARCH=arm64,AVAILABLE_DIST=centos7,DIST=centos7,MACHINE_SIZE=huge/2037/consoleFull

When I looked, it had the lack of intrinsics:

-- Performing Test HAVE_ARMV8_CRC_CRYPTO_INTRINSICS - Failed

and successfully built the crc file as a result:

Building C object src/CMakeFiles/common_crc_aarch64.dir/common/crc32c_aarch64.c.o
Linking C static library ../lib/libcommon_crc_aarch64.a
[ 1%] Built target common_crc_aarch64

Member

dmick commented Mar 25, 2017

Actual test build here:

https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=arm64,AVAILABLE_ARCH=arm64,AVAILABLE_DIST=centos7,DIST=centos7,MACHINE_SIZE=huge/2037/consoleFull

When I looked, it had the lack of intrinsics:

-- Performing Test HAVE_ARMV8_CRC_CRYPTO_INTRINSICS - Failed

and successfully built the crc file as a result:

Building C object src/CMakeFiles/common_crc_aarch64.dir/common/crc32c_aarch64.c.o
Linking C static library ../lib/libcommon_crc_aarch64.a
[ 1%] Built target common_crc_aarch64

#include <inttypes.h>
int main() { uint32_t a; uint8_t b; __builtin_aarch64_crc32b(a, b); }
" HAVE_ARMV8_CRC_CRYPTO_INTRINSICS)
endif()

This comment has been minimized.

@tchaikov

tchaikov Mar 25, 2017

Contributor

makes sense to me.

@tchaikov

tchaikov Mar 25, 2017

Contributor

makes sense to me.

This comment has been minimized.

@tchaikov

tchaikov Mar 25, 2017

Contributor

as "-m" does not imply that the compiler supports the intrinsics.

@tchaikov

tchaikov Mar 25, 2017

Contributor

as "-m" does not imply that the compiler supports the intrinsics.

This comment has been minimized.

@dmick

dmick Mar 25, 2017

Member

oh...I guess that's true, huh; it only indicates that the compiler can output the instructions. Hm. Maybe this shouldn't even bother with -march

@dmick

dmick Mar 25, 2017

Member

oh...I guess that's true, huh; it only indicates that the compiler can output the instructions. Hm. Maybe this shouldn't even bother with -march

This comment has been minimized.

@tchaikov

tchaikov Mar 25, 2017

Contributor

but do we need to detect the support of -march=armv8-a+crc+crypto before using the crc32 intrinsics ? IIUC, -m<machine> instructs the compiler backend to emit instructions offered by that $machine, but in this case, we are not passing -march=armv8-a+crc+crypto to achieve this goal, instead we manually optimize the crc32 digest algorithm. so i guess we can just drop CHECK_C_COMPILER_FLAG(-march=armv8-a+crc+crypto HAVE_ARMV8_CRC_CRYPTO_INTRINSICS_MARCH), and detect HAVE_ARMV8_CRC_CRYPTO_INTRINSICS using check_cxx_source_compiles(...).

@tchaikov

tchaikov Mar 25, 2017

Contributor

but do we need to detect the support of -march=armv8-a+crc+crypto before using the crc32 intrinsics ? IIUC, -m<machine> instructs the compiler backend to emit instructions offered by that $machine, but in this case, we are not passing -march=armv8-a+crc+crypto to achieve this goal, instead we manually optimize the crc32 digest algorithm. so i guess we can just drop CHECK_C_COMPILER_FLAG(-march=armv8-a+crc+crypto HAVE_ARMV8_CRC_CRYPTO_INTRINSICS_MARCH), and detect HAVE_ARMV8_CRC_CRYPTO_INTRINSICS using check_cxx_source_compiles(...).

This comment has been minimized.

@dmick

dmick Mar 25, 2017

Member

Maybe?....I dunno what happens if you try to refer to the intrinsic but do not pass crc+crypto.

@dmick

dmick Mar 25, 2017

Member

Maybe?....I dunno what happens if you try to refer to the intrinsic but do not pass crc+crypto.

This comment has been minimized.

@dmick

dmick Mar 25, 2017

Member

Answer: if you don't pass -march=armv8-a+crc, you get an error from assembly.

$ cat test.c
#include <inttypes.h>
int main() { uint32_t a; uint8_t b; __builtin_aarch64_crc32b(a, b); }

$ gcc test.c -o test
test.c: In function ‘main’:
test.c:2:1: error: unrecognizable insn:
 int main() { uint32_t a; uint8_t b; __builtin_aarch64_crc32b(a, b); }
 ^
(insn 7 6 8 2 (set (reg:SI 75)
        (unspec:SI [
                (reg:SI 76)
                (reg:QI 77)
            ] UNSPEC_CRC32B)) test.c:2 -1
     (nil))
test.c:2:1: internal compiler error: in extract_insn, at recog.c:2343

$ gcc -march=armv8-a+crc test.c -o test
$

So I guess we need both tests.

@dmick

dmick Mar 25, 2017

Member

Answer: if you don't pass -march=armv8-a+crc, you get an error from assembly.

$ cat test.c
#include <inttypes.h>
int main() { uint32_t a; uint8_t b; __builtin_aarch64_crc32b(a, b); }

$ gcc test.c -o test
test.c: In function ‘main’:
test.c:2:1: error: unrecognizable insn:
 int main() { uint32_t a; uint8_t b; __builtin_aarch64_crc32b(a, b); }
 ^
(insn 7 6 8 2 (set (reg:SI 75)
        (unspec:SI [
                (reg:SI 76)
                (reg:QI 77)
            ] UNSPEC_CRC32B)) test.c:2 -1
     (nil))
test.c:2:1: internal compiler error: in extract_insn, at recog.c:2343

$ gcc -march=armv8-a+crc test.c -o test
$

So I guess we need both tests.

This comment has been minimized.

@tchaikov

tchaikov Mar 25, 2017

Contributor

i dunno what happens if you try to refer to the intrinsic but do not pass crc+crypto.

the intrinsics are implemented using the the GCC builtin functions like __builtin_aarch64_crc32b() which is directly mapped to the corresponding assembly without the help of the -m<machine>. (i skimmed https://github.com/gcc-mirror/gcc/blob/master/gcc/config/aarch64/aarch64-builtins.c, and got this conclusion =))

so i guess we can just drop CHECK_C_COMPILER_FLAG(-march=armv8-a+crc+crypto HAVE_ARMV8_CRC_CRYPTO_INTRINSICS_MARCH)

okay, we cannot drop it, as we are trying to set ARMV8_CRC_COMPILE_FLAGS if -march=armv8-a+crc+crypto is supported. so maybe we can s/HAVE_ARMV8_CRC_CRYPTO_INTRINSICS/HAVE_ARMV8_CRC_CRYPTO_INTRINSICS_MARCH/at line 59. and maybe renameHAVE_ARMV8_CRC_CRYPTO_INTRINSICS_MARCHtoHAVE_ARMV8_CRC_CRYPTO_MARCH`.

@tchaikov

tchaikov Mar 25, 2017

Contributor

i dunno what happens if you try to refer to the intrinsic but do not pass crc+crypto.

the intrinsics are implemented using the the GCC builtin functions like __builtin_aarch64_crc32b() which is directly mapped to the corresponding assembly without the help of the -m<machine>. (i skimmed https://github.com/gcc-mirror/gcc/blob/master/gcc/config/aarch64/aarch64-builtins.c, and got this conclusion =))

so i guess we can just drop CHECK_C_COMPILER_FLAG(-march=armv8-a+crc+crypto HAVE_ARMV8_CRC_CRYPTO_INTRINSICS_MARCH)

okay, we cannot drop it, as we are trying to set ARMV8_CRC_COMPILE_FLAGS if -march=armv8-a+crc+crypto is supported. so maybe we can s/HAVE_ARMV8_CRC_CRYPTO_INTRINSICS/HAVE_ARMV8_CRC_CRYPTO_INTRINSICS_MARCH/at line 59. and maybe renameHAVE_ARMV8_CRC_CRYPTO_INTRINSICS_MARCHtoHAVE_ARMV8_CRC_CRYPTO_MARCH`.

This comment has been minimized.

@tchaikov

tchaikov Mar 25, 2017

Contributor

@dmick thanks for confirming this! should have tested it. but could you consider the second suggestion in the above comment?

@tchaikov

tchaikov Mar 25, 2017

Contributor

@dmick thanks for confirming this! should have tested it. but could you consider the second suggestion in the above comment?

This comment has been minimized.

@dmick

dmick Mar 27, 2017

Member

I think it makes the most sense to use HAVE_ARMV8_CRC_CRYPTO_MARCH and HAVE_ARMV8_CRC_CRYPTO_INTRINSICS. It's true we can't use the second if we don't have the first, but we're testing for them separately. But I agree it makes it better to have the first named more accurately.

@dmick

dmick Mar 27, 2017

Member

I think it makes the most sense to use HAVE_ARMV8_CRC_CRYPTO_MARCH and HAVE_ARMV8_CRC_CRYPTO_INTRINSICS. It's true we can't use the second if we don't have the first, but we're testing for them separately. But I agree it makes it better to have the first named more accurately.

@liewegas liewegas changed the title from Fix cmake's ARM CRC intrinsics test to handle duplicitous gcc 4.8.5 to arch: fix cmake's ARM CRC intrinsics test to handle duplicitous gcc 4.8.5 Mar 25, 2017

cmake/modules/SIMDExt.cmake: armv8 crypto intrinsics
Test not only for -march support, but also the actual
presence of the intrinsic routines.  Not sure why, but gcc
4.8.5 passes the first but not the second.

Fixes: http://tracker.ceph.com/issues/19386
Signed-off-by: Dan Mick <dan.mick@redhat.com>
@dmick

This comment has been minimized.

Show comment
Hide comment
@dmick
Member

dmick commented Mar 27, 2017

@dmick dmick merged commit 64ee8e8 into ceph:master Mar 27, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@dmick dmick deleted the dmick:wip-arm-crc branch Mar 27, 2017

@theanalyst theanalyst changed the title from arch: fix cmake's ARM CRC intrinsics test to handle duplicitous gcc 4.8.5 to build/ops: arch: fix cmake's ARM CRC intrinsics test to handle duplicitous gcc 4.8.5 (issue#19386, pr#14132, Dan Mick) Apr 18, 2017

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