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 18: (8 comments) > Could you separate all third-party files imported as-is into a > separate change list and then rebase this patch on top of that? > > Having its own changelist for those third-party files will help to > separate the changes in the Kudu's and third-party code to support > Kudu building and running on aarch64. Thanks a lot for your review. reply inline, OK, I will make the 3rd party importted files into a separated patch. http://gerrit.cloudera.org:8080/#/c/14964/18//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14964/18//COMMIT_MSG@9 PS18, Line 9: This change try to modify and make Kudu can build sucessfully on both : x86_64 and aarch64 platform. > How about rephrasing it a bit: thanks, done http://gerrit.cloudera.org:8080/#/c/14964/18/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/14964/18/build-support/jenkins/build-and-test.sh@207 PS18, Line 207: all of test cases failed > all tests fail Done http://gerrit.cloudera.org:8080/#/c/14964/18/build-support/jenkins/build-and-test.sh@208 PS18, Line 208: so apply static linking direcly > rephrase for clarity: Done http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/common/columnar_serialization.cc File src/kudu/common/columnar_serialization.cc: http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/common/columnar_serialization.cc@22 PS18, Line 22: //__aarch64__ > nit: drop this comment, it doesn't add much value and even a bit confusing, Done http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/common/key_encoder.h File src/kudu/common/key_encoder.h: http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/common/key_encoder.h@286 PS18, Line 286: #else : return false; : #endif //__aarch64__ > nit: from the readability perspective, I think it will be better to have th Thanks, done http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/gutil/atomicops-internals-x86.cc File src/kudu/gutil/atomicops-internals-x86.cc: http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/gutil/atomicops-internals-x86.cc@25 PS18, Line 25: #if defined(__i386__) || defined(__x86_64__) > Instead, is it possible to exclude this file from compilation at cmake leve agree, thank you, done http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/gutil/dynamic_annotations.h File src/kudu/gutil/dynamic_annotations.h: http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/gutil/dynamic_annotations.h@60 PS18, Line 60: #include <stddef.h> > nit: consider instead I have tried to change according to your suggestion, but it raised following error in building: In file included from /home/kudu/kudu/src/kudu/gutil/dynamic_annotations.c:41:0: /home/kudu/kudu/src/kudu/gutil/dynamic_annotations.h:60:10: fatal error: cstddef: No such file or directory #include <cstddef> ^~~~~~~~~ compilation terminated. http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/util/init.cc File src/kudu/util/init.cc: http://gerrit.cloudera.org:8080/#/c/14964/18/src/kudu/util/init.cc@69 PS18, Line 69: if (!cpu.has_broken_neon() && cpu.cpu_brand()=="ARM64") { > Could you add a comment explaining because ARM64 doesn't support SSE instructions, and there is a conversion between SSE and NEON on ARM, see see2neon.h. so here we just check if the host is ARM platform and support NEON. the cpu_brand "ARM64" is because on oter platforms it can be directly get from /pro/cpuinfo, but on ARM the cpu_branch cannot be directly query. so we just assigne it to "ARM64" to keep consistent. please see L280 of https://gerrit.cloudera.org/#/c/14964/18/src/kudu/gutil/cpu.cc but here seems it is not a good iead to check by using cpu_brand, I will try to find a better way. -- 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: 18 Gerrit-Owner: liusheng <[email protected]> Gerrit-Reviewer: Adar Lieber-Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: RuiChen <[email protected]> Gerrit-Reviewer: liusheng <[email protected]> Gerrit-Comment-Date: Thu, 30 Apr 2020 03:37:28 +0000 Gerrit-HasComments: Yes
