Alexey Serbin 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) 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; > 1. We can't get cpu brand like x86 or from 'proc/cpuinfo' for aarch64 serve Maybe, the approach used for aarm64 in https://github.com/google/cpu_features could give some useful insights? http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/util/block_bloom_filter.h File src/kudu/util/block_bloom_filter.h: http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/util/block_bloom_filter.h@20 PS7, Line 20: #ifndef __aarch64__ Why is this conditional for the <algorithm> header? This looks a bit strange because std::max<>() is used in this file (block_bloom_filter.h) regardless of the architecture. http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/util/debug-util.h File src/kudu/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/util/debug-util.h@34 PS7, Line 34: #define FUTEX_WAIT 0 : #define FUTEX_WAKE 1 : #define FUTEX_PRIVATE_FLAG 128 Where are these used and are they architecture-specific? Could you add a comment for these macros? http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/util/sse2neon.h File src/kudu/util/sse2neon.h: http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/util/sse2neon.h@20 PS7, Line 20: /* : * The MIT license: : * : * Permission is hereby granted, free of charge, to any person obtaining a copy : * of this software and associated documentation files (the "Software"), to deal : * in the Software without restriction, including without limitation the rights : * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell : * copies of the Software, and to permit persons to whom the Software is : * furnished to do so, subject to the following conditions: : * : * The above copyright notice and this permission notice shall be included in : * all copies or substantial portions of the Software. : * : * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR : * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, : * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE : * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER : * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, : * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE : * SOFTWARE. : */ nit: could you move this to the very top of the file? I think the RAT license checker would complain if it doesn't find the license header in the very top. http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/util/striped64.cc File src/kudu/util/striped64.cc: http://gerrit.cloudera.org:8080/#/c/14964/7/src/kudu/util/striped64.cc@22 PS7, Line 22: #define _mm_malloc(a, b) malloc(a) Does this work for any type of alignment? Should we do something with custom alignment, if requested? I'm not an expert there, but found this link which might be relevant: https://github.com/clab/dynet/issues/266 -- 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 <liusheng2...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <huangtianhua...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: liusheng <liusheng2...@gmail.com> Gerrit-Comment-Date: Tue, 07 Jan 2020 19:35:19 +0000 Gerrit-HasComments: Yes
