Copilot commented on code in PR #3596:
URL: https://github.com/apache/brpc/pull/3596#discussion_r4240162822
##########
test/CMakeLists.txt:
##########
@@ -202,9 +205,29 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
)
endif()
+# Read module-owned sources from their targets, not parent-scope variables.
+get_target_property(BUTIL_SOURCES BUTIL_LIB SOURCES)
+# Source properties are directory-scoped; reuse production CRC32C settings.
+get_target_property(butil_crc32c_options BUTIL_LIB BRPC_CRC32C_COMPILE_OPTIONS)
+if(butil_crc32c_options)
+ set_property(SOURCE ${CMAKE_CURRENT_SOURCE_DIR}/../src/butil/crc32c.cc
+ APPEND PROPERTY COMPILE_OPTIONS ${butil_crc32c_options})
+endif()
+if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND
+ CMAKE_SYSTEM_PROCESSOR MATCHES "riscv64" AND WITH_RISCV_ZVBC)
+ set_property(SOURCE
${CMAKE_CURRENT_SOURCE_DIR}/../src/butil/third_party/modp_b64/modp_b64_rvv.cc
+ APPEND PROPERTY COMPILE_OPTIONS -march=rv64gcv_zbc_zvbc)
Review Comment:
The debug-library copy of the RISC-V settings repeats the same partial flag
assignment: only `modp_b64_rvv.cc` gets `-march`, while its caller and the
IOBuf/StringPiece RVV sources are compiled as scalar code. Apply the Zvbc
option consistently to all RVV-dependent entries in `BUTIL_SOURCES` so the
debug build matches production and can exercise these paths.
##########
src/butil/CMakeLists.txt:
##########
@@ -0,0 +1,351 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+set(WITH_GLOG_VAL "0")
+if(WITH_GLOG)
+ set(WITH_GLOG_VAL "1")
+ set(BRPC_WITH_GLOG 1)
+endif()
+
+set(WITH_CPU_FREQUENCY_VAL "0")
+if(WITH_CPU_FREQUENCY)
+ set(WITH_CPU_FREQUENCY_VAL "1")
+ set(BUTIL_USE_CPU_FREQUENCY 1)
+endif()
+
+set(BUTIL_DEFINITIONS
+ BRPC_WITH_GLOG=${WITH_GLOG_VAL}
+ BUTIL_USE_CPU_FREQUENCY=${WITH_CPU_FREQUENCY_VAL})
+if(WITH_MESALINK)
+ list(APPEND BUTIL_DEFINITIONS USE_MESALINK)
+endif()
+if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
+ include(CheckFunctionExists)
+ CHECK_FUNCTION_EXISTS(clock_gettime HAVE_CLOCK_GETTIME)
+ if(NOT HAVE_CLOCK_GETTIME)
+ list(APPEND BUTIL_DEFINITIONS NO_CLOCK_GETTIME_IN_MAC)
+ endif()
+endif()
+
+configure_file(${CMAKE_CURRENT_SOURCE_DIR}/config.h.in
+ ${CMAKE_CURRENT_SOURCE_DIR}/config.h @ONLY)
+
+set(BUTIL_INCLUDE_DIRS ${GFLAGS_INCLUDE_PATH} ${PROTOBUF_INCLUDE_DIRS})
+if(WITH_GLOG)
+ find_path(GLOG_INCLUDE_PATH NAMES glog/logging.h)
+ find_library(GLOG_LIB NAMES glog)
+ if((NOT GLOG_INCLUDE_PATH) OR (NOT GLOG_LIB))
+ message(FATAL_ERROR "Fail to find glog")
+ endif()
+ list(APPEND BUTIL_INCLUDE_DIRS ${GLOG_INCLUDE_PATH})
+endif()
+
+if(WITH_MESALINK)
+ find_path(MESALINK_INCLUDE_PATH NAMES mesalink/openssl/ssl.h)
+ find_library(MESALINK_LIB NAMES mesalink)
+ if((NOT MESALINK_INCLUDE_PATH) OR (NOT MESALINK_LIB))
+ message(FATAL_ERROR "Fail to find MesaLink")
+ else()
+ message(STATUS "Found MesaLink: ${MESALINK_LIB}")
+ endif()
+ list(APPEND BUTIL_INCLUDE_DIRS ${MESALINK_INCLUDE_PATH})
+endif()
+
+if(WITH_BORINGSSL)
+ find_package(BoringSSL REQUIRED)
+ list(APPEND BUTIL_INCLUDE_DIRS ${BORINGSSL_INCLUDE_DIR})
+else()
+ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND NOT OPENSSL_ROOT_DIR)
+ set(OPENSSL_ROOT_DIR
+ "/usr/local/opt/openssl" # Homebrew installed OpenSSL
+ )
+ endif()
+
+ find_package(OpenSSL REQUIRED)
+ list(APPEND BUTIL_INCLUDE_DIRS ${OPENSSL_INCLUDE_DIR})
+endif()
+
+add_library(butil_build_config INTERFACE)
+target_link_libraries(butil_build_config INTERFACE brpc_common_config)
+target_compile_definitions(butil_build_config INTERFACE ${BUTIL_DEFINITIONS})
+target_include_directories(butil_build_config INTERFACE ${BUTIL_INCLUDE_DIRS})
+
+set(butil_crc32c_options)
+if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
+ if(CMAKE_SYSTEM_PROCESSOR MATCHES "(x86)|(X86)|(amd64)|(AMD64)")
+ # Only CRC32C needs the SSE4.2 intrinsics and feature macro.
+ list(APPEND butil_crc32c_options -msse4.2)
+ elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "riscv64")
+ option(WITH_RISCV_ZBC "Enable RISC-V Zbc carry-less multiplication for
CRC32C acceleration" OFF)
+ option(WITH_RISCV_ZVBC "Enable RISC-V Zvbc vector carry-less
multiplication for CRC32C acceleration" OFF)
+ if(WITH_RISCV_ZVBC)
+ list(APPEND butil_crc32c_options -march=rv64gcv_zbc_zvbc)
+ # Preserve the existing RVV base64 implementation as well.
+ set_property(SOURCE
${CMAKE_CURRENT_SOURCE_DIR}/third_party/modp_b64/modp_b64_rvv.cc
+ APPEND PROPERTY COMPILE_OPTIONS -march=rv64gcv_zbc_zvbc)
Review Comment:
With `WITH_RISCV_ZVBC=ON`, the new scoping applies the vector ISA only to
CRC32C and `modp_b64_rvv.cc`. The callers in `modp_b64.cc`, `iobuf.cpp`, and
`strings/string_piece.cc`, plus the `rvv_memcmp` implementation in
`string_compare_rvv.cc`, are therefore compiled without `__riscv_vector`; the
RVV paths are compiled out (and vector-enabled downstream `StringPiece` code
can reference symbols the library did not emit). Apply this option to every
RVV-dependent source, or retain the previous target-wide `-march` behavior, and
mirror that setup for the debug objects in `test/CMakeLists.txt`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
