liusheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/14964 )
Change subject: KUDU-3007. Support building Kudu on aarch64 platform ...................................................................... Patch Set 7: (5 comments) Hi Adar, Thanks for your review! I thought to make the building process sucessfully to be the first step, ane then fix and make the unit tests passed. sorry, my fault, I wll try to run unitests on aarch64 and share the results here. http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/cfile/binary_plain_block.cc File src/kudu/cfile/binary_plain_block.cc: http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/cfile/binary_plain_block.cc@202 PS7, Line 202: #ifndef __aarch64__ : p = coding::DecodeGroupVarInt32_SSE( : p, &dst_ptr[0], &dst_ptr[1], &dst_ptr[2], &dst_ptr[3]); : #endif //__aarch64__ > This seems wrong; shouldn't we call the SlowButSafe function below? Meaning Hmm, sorry my mistake, we need to move these to L211 http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/cfile/bitshuffle_arch_wrapper.cc File src/kudu/cfile/bitshuffle_arch_wrapper.cc: http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/cfile/bitshuffle_arch_wrapper.cc@60 PS7, Line 60: #if !defined(__APPLE__) && !defined(__aarch64__) > We can't safely call CPU().has_avx2() to determine this on aarch64? Why not yes, the CPU().has_avx2() is false on aarch64, but if we don't add this macro condition, the L61~L64 will still need to be compiled and following error will occur, because the if condition statement cannot effect compile process ../../../lib/libcfile.so: error: undefined reference to 'bshuf_compress_lz4_bound_avx2' ../../../lib/libcfile.so: error: undefined reference to 'bshuf_compress_lz4_avx2' ../../../lib/libcfile.so: error: undefined reference to 'bshuf_decompress_lz4_avx2 http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/common/key_encoder.h File src/kudu/common/key_encoder.h: http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/common/key_encoder.h@249 PS7, Line 249: #ifndef __aarch64__ > Shouldn't the aarch64 case return false so we go through the slow loop? yes, it looks like here should return false, thanks http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/gutil/linux_syscall_support.h File src/kudu/gutil/linux_syscall_support.h: PS7: > What caused the changes in this file? when I tried to build Kudu on ARM server, there are some errors occured due to this file, it looks like this code file didn't support aarch64 previously, and the newest implementation now can support ARM, so I just update this from: https://github.com/alk/gperftools/blob/master/src/base/linux_syscall_support.h How do you think about ? http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/gutil/spinlock.h File src/kudu/gutil/spinlock.h: PS7: > I don't really understand the changes here. it is because the spinlock on ARM server will cause deadlock, I will try to find more details to explain. -- To view, visit http://gerrit.cloudera.org:8080/14964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2953519c5d28de17e6b2bb7094abab0c1cd12c97 Gerrit-Change-Number: 14964 Gerrit-PatchSet: 7 Gerrit-Owner: liusheng <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: liusheng <[email protected]> Gerrit-Comment-Date: Tue, 07 Jan 2020 10:09:07 +0000 Gerrit-HasComments: Yes
