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

cmake,crc32c: conditionalize crc32c on different archs #14289

Merged
merged 3 commits into from Apr 4, 2017

Conversation

Projects
None yet
4 participants
@tchaikov
Contributor

tchaikov commented Apr 2, 2017

No description provided.

tchaikov added some commits Apr 2, 2017

cmake: extract ppc64le detection into SIMDExt.cmake
Signed-off-by: Kefu Chai <kchai@redhat.com>
cmake: point to asm_compiler to yasm only if HAVE_INTEL
as currently only isa uses it.

Signed-off-by: Kefu Chai <kchai@redhat.com>
@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Apr 2, 2017

@tchaikov
I see at least bin/unittest_crc32c complain about missing ARM CRC.

Perhaps solve all the include stuff in include/crc32c.h

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 3, 2017

@wjwithagen fixed and repushed.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Apr 3, 2017

@tchaikov
LGTM

@tchaikov tchaikov requested review from dmick and cbodley Apr 3, 2017

@cbodley

cbodley approved these changes Apr 3, 2017

cmake bits look right, but i don't have an environment to test it

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 3, 2017

@kestrels could you help take a look at this change also?

@kestrels

This comment has been minimized.

Contributor

kestrels commented Apr 3, 2017

Sure.

@kestrels

This comment has been minimized.

Contributor

kestrels commented Apr 3, 2017

This is hitting a build break for me on my ppc64le system. The break is coming from src/common/simple_spin.cc.

[ 40%] Building CXX object src/librbd/CMakeFiles/rbd_internal.dir/image/CreateRequest.cc.o
[ 40%] Building CXX object src/CMakeFiles/common-objs.dir/common/code_environment.cc.o
[ 40%] Building CXX object src/CMakeFiles/common-objs.dir/common/dout.cc.o
[ 40%] Building CXX object src/CMakeFiles/common-objs.dir/common/signal.cc.o
[ 40%] Building CXX object src/CMakeFiles/common-objs.dir/common/simple_spin.cc.o
/home/ubuntu/ceph/tchaikov.1/ceph/src/common/simple_spin.cc:38:2: error: #error "Unknown architecture"
#error "Unknown architecture"
^
src/CMakeFiles/common-objs.dir/build.make:2415: recipe for target 'src/CMakeFiles/common-objs.dir/common/simple_spin.cc.o' failed
make[2]: *** [src/CMakeFiles/common-objs.dir/common/simple_spin.cc.o] Error 1
CMakeFiles/Makefile2:686: recipe for target 'src/CMakeFiles/common-objs.dir/all' failed
make[1]: *** [src/CMakeFiles/common-objs.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 40%] Building CXX object src/mds/CMakeFiles/mds.dir/CDir.cc.o
[ 40%] Building CXX object src/librbd/CMakeFiles/rbd_internal.dir/image/OpenRequest.cc.o
[ 40%] Building CXX object src/mds/CMakeFiles/mds.dir/CInode.cc.o
[ 40%] Building CXX object src/mds/CMakeFiles/mds.dir/LogEvent.cc.o

The ppc64 is not defined. It should probably be changed to powerpc or powerpc64, both of which are defined.

if(HAVE_ARM)
list(APPEND libcommon_files arch/arm.c)
elseif(HAVE_INTEL)
list(APPEND libcommon_files arch/intel.c)

This comment has been minimized.

@kestrels

kestrels Apr 4, 2017

Contributor

There are symbols defined in src/arch/intel.c (e.g. ceph_arch_intel_pclmul and ceph_arch_intel_sse41) that are referenced in other parts of the code outside of any ifdef.

I think the answer is probably to either (a) go back to unconditionally having arch/intel.c unconditionally (even on non-intel compiles) or (b) go forward with finding all references to those symbols and hiding them behind the proper intel related ifdefs.

[100%] ?[32m?[1mLinking CXX executable ../../bin/radosgw-admin?[0m
[100%] Built target radosgw-admin
../../../lib/libcephd.a(libcephd.cc.o):(.toc+0x10): undefined reference to ceph_arch_intel_pclmul' ../../../lib/libcephd.a(libcephd.cc.o):(.toc+0x18): undefined reference to ceph_arch_intel_sse41'
collect2: error: ld returned 1 exit status
src/test/libcephd/CMakeFiles/ceph_test_cephd_api_misc.dir/build.make:147: recipe for target 'bin/ceph_test_cephd_api_misc' failed
make[2]: *** [bin/ceph_test_cephd_api_misc] Error 1
CMakeFiles/Makefile2:13507: recipe for target 'src/test/libcephd/CMakeFiles/ceph_test_cephd_api_misc.dir/all' failed
make[1]: *** [src/test/libcephd/CMakeFiles/ceph_test_cephd_api_misc.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[100%] ?[32m?[1mLinking CXX executable ../bin/ceph-dencoder?[0m
[100%] Built target ceph-dencoder

~/ceph/tchaikov.1/ceph/src/compressor/zlib$ cat CompressionPluginZlib.h | grep intel
#include "arch/intel.h"
isal = (ceph_arch_intel_pclmul && ceph_arch_intel_sse41);
~/ceph/tchaikov.1/ceph/src/compressor/zlib$

crc32c,compressor: only compile functions on supported arch
so we can still compile if certain header file (<sys/auxv.h>) used
for detecting cpu capabilities on ppc is missing on x86 host machine.

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 4, 2017

@kestrels thanks for testing the change! the second build failure is addressed and the fix is re-pushed.

@kestrels

This comment has been minimized.

Contributor

kestrels commented Apr 4, 2017

With both #14289 and #14310, the build completed successfully. The unittest_crc32c confirmed that the ppc specific crc algorithm is being used.

@tchaikov tchaikov merged commit ffea115 into ceph:master Apr 4, 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

@tchaikov tchaikov deleted the tchaikov:wip-conditionalize-crc32c branch Apr 4, 2017

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