Adar Dembo 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: (6 comments) I started to review this but stopped; the issues I found suggest that the Kudu unit tests can't pass on aarch64. If that's true, it'll be pretty tough to review any of this; how do we know that the changes are correct? 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, shouldn't the ifdef be built into the condition on L201 in some way? 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? 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? http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/gutil/cpu.cc File src/kudu/gutil/cpu.cc: http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/gutil/cpu.cc@271 PS7, Line 271: has_broken_neon_ = false; Isn't this the default value? 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? 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. -- 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: Kudu Jenkins (120) Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: liusheng <[email protected]> Gerrit-Comment-Date: Mon, 06 Jan 2020 06:05:00 +0000 Gerrit-HasComments: Yes
