tkonolige commented on a change in pull request #8822:
URL: https://github.com/apache/tvm/pull/8822#discussion_r694386210
##########
File path: cmake/modules/Hexagon.cmake
##########
@@ -116,26 +85,15 @@ if(USE_HEXAGON_DEVICE STREQUAL "${PICK_SIM}")
INSTALL_COMMAND "true"
)
elseif(USE_HEXAGON_DEVICE STREQUAL "${PICK_HW}")
- find_hexagon_sdk_root()
+ find_hexagon_sdk_root("${USE_HEXAGON_SDK}" "v66")
find_hexagon_toolchain()
- message(STATUS "Hexagon SDK: ${HEXAGON_SDK_ROOT}")
- if(HEXAGON_SDK_ROOT MATCHES "3.5.1")
- message(SEND_ERROR "Hexagon SDK 3.5.1 is not supported")
- elseif(HEXAGON_SDK_ROOT MATCHES "3\.[0-9]+\.[0-9]+")
- set(RPCMEM_DIR "libs/common/rpcmem")
- set(REMOTE_DIR "libs/common/remote/ship/android_Release_aarch64")
- else()
- set(RPCMEM_DIR "ipc/fastrpc/rpcmem")
- set(REMOTE_DIR "ipc/fastrpc/remote/ship/android_aarch64")
- endif()
file(GLOB RUNTIME_HEXAGON_DEVICE_SRCS src/runtime/hexagon/target/*.cc)
- include_directories(SYSTEM "${HEXAGON_SDK_ROOT}/incs/stddef")
- include_directories(SYSTEM "${HEXAGON_SDK_ROOT}/${RPCMEM_DIR}/inc")
- include_directories(
- SYSTEM "${HEXAGON_SDK_ROOT}/incs")
- include_directories(
- SYSTEM "${HEXAGON_SDK_ROOT}/${REMOTE_DIR}")
- include_directories(SYSTEM "${HEXAGON_TOOLCHAIN}/include/iss")
+
+ include_directories(SYSTEM
+ ${HEXAGON_SDK_INCLUDES}
+ ${HEXAGON_RPCMEM_ROOT}/inc
+ ${HEXAGON_REMOTE_ROOT}
+ )
Review comment:
Can you also switch this to `target_include_directories`?
##########
File path: cmake/modules/Hexagon.cmake
##########
@@ -47,41 +47,10 @@ function(find_hexagon_toolchain)
endif()
endfunction()
-function(find_hexagon_sdk_root)
- if(FOUND_HEXAGON_SDK_ROOT)
- return()
- endif()
- message(STATUS "Checking Hexagon SDK root: ${USE_HEXAGON_SDK}")
- file(GLOB_RECURSE HEXAGON_AEESTDDEF "${USE_HEXAGON_SDK}/*/AEEStdDef.h")
- if(HEXAGON_AEESTDDEF)
- # The path is ${HEXAGON_SDK_ROOT}/incs/stddef/AEEStdDef.h.
- get_filename_component(HEXAGON_TMP0 "${HEXAGON_AEESTDDEF}" DIRECTORY)
- get_filename_component(HEXAGON_TMP1 "${HEXAGON_TMP0}" DIRECTORY)
- get_filename_component(HEXAGON_TMP2 "${HEXAGON_TMP1}" DIRECTORY)
- set(HEXAGON_SDK_ROOT "${HEXAGON_TMP2}" CACHE PATH
- "Root directory of Hexagon SDK")
- set(FOUND_HEXAGON_SDK_ROOT TRUE)
- else(HEXAGON_AEESTDDEF)
- message(SEND_ERROR "Cannot validate Hexagon SDK in ${USE_HEXAGON_SDK}")
- endif()
-endfunction()
-
if(BUILD_FOR_HEXAGON)
- find_hexagon_sdk_root()
- if(HEXAGON_SDK_ROOT MATCHES "3.5.1")
- message(SEND_ERROR "Hexagon SDK 3.5.1 is not supported")
- elseif(HEXAGON_SDK_ROOT MATCHES "3\.[0-9]+\.[0-9]+")
- include_directories(
- SYSTEM "${USE_HEXAGON_SDK}/libs/common/qurt/ADSPv62MP/include/posix"
- SYSTEM "${USE_HEXAGON_SDK}/libs/common/qurt/ADSPv62MP/include/qurt")
- else()
- include_directories(
- SYSTEM "${HEXAGON_SDK_ROOT}/rtos/qurt/computev65/include/posix"
- SYSTEM "${HEXAGON_SDK_ROOT}/rtos/qurt/computev65/include/qurt")
- endif()
- include_directories(
- SYSTEM "${HEXAGON_SDK_ROOT}/incs"
- SYSTEM "${HEXAGON_SDK_ROOT}/incs/stddef")
+ find_hexagon_sdk_root("${USE_HEXAGON_SDK}" "v66")
+ # Add SDK and QuRT includes when building for Hexagon.
+ include_directories(SYSTEM ${HEXAGON_SDK_INCLUDES} ${HEXAGON_QURT_INCLUDES})
Review comment:
Can you switch this to `target_include_directories`? You may have to
call it multiple times with different targets (`tvm_objs`, `tvm`). I know the
old code used `include_directories`, but it would be good to clean this up.
##########
File path: src/runtime/hexagon/target/fastrpc/CMakeLists.txt
##########
@@ -70,81 +67,66 @@ set(TVM_REMOTE_D_SKEL_C "tvm_remote_skel.c")
set(TVM_REMOTE_D_STUB_C "tvm_remote_stub.c")
add_custom_command(
- OUTPUT ${TVM_REMOTE_D_SKEL_C} ${TVM_REMOTE_D_STUB_C}
- "${FASTRPC_SRC}/include/${TVM_REMOTE_D_H}"
- COMMAND ${QAIC_EXE} ${QAIC_FLAGS}
- "${FASTRPC_SRC}/include/${TVM_REMOTE_D_IDL}"
- COMMAND ${CMAKE_COMMAND} -E rename "${TVM_REMOTE_D_H}"
- "${FASTRPC_SRC}/include/${TVM_REMOTE_D_H}"
- MAIN_DEPENDENCY "${FASTRPC_SRC}/include/${TVM_REMOTE_D_IDL}"
+ OUTPUT ${TVM_REMOTE_D_SKEL_C} ${TVM_REMOTE_D_STUB_C}
+ "${FASTRPC_SRC}/include/${TVM_REMOTE_D_H}"
+ COMMAND ${QAIC_EXE} ${QAIC_FLAGS}
+ "${FASTRPC_SRC}/include/${TVM_REMOTE_D_IDL}"
+ COMMAND ${CMAKE_COMMAND} -E rename "${TVM_REMOTE_D_H}"
+ "${FASTRPC_SRC}/include/${TVM_REMOTE_D_H}"
+ MAIN_DEPENDENCY "${FASTRPC_SRC}/include/${TVM_REMOTE_D_IDL}"
)
if("${FASTRPC_LIBS}" STREQUAL "SKEL")
# Skel libraries.
#
- set(HEXARCH_DIR_v60 "ADSPv60MP")
- set(HEXARCH_DIR_v62 "ADSPv62MP")
- set(HEXARCH_DIR_v65 "computev65")
- set(HEXARCH_DIR_v66 "computev66")
- set(HEXARCH_DIR_STR "HEXARCH_DIR_${HEXAGON_ARCH}")
- set(HEXARCH_DIR ${${HEXARCH_DIR_STR}})
-
- if(NOT HEXARCH_DIR)
- message(SEND_ERROR
- "Please set HEXAGON_ARCH to one of v60, v62, v65, v66")
- endif()
-
- include_directories(
- SYSTEM ${HEXAGON_SDK_ROOT}/libs/common/qurt/${HEXARCH_DIR}/include/qurt)
- include_directories(
- SYSTEM ${HEXAGON_SDK_ROOT}/libs/common/qurt/${HEXARCH_DIR}/include/posix)
+ include_directories(SYSTEM ${HEXAGON_QURT_INCLUDES})
# Extra compile flags (both C and C++).
set(EXTRA_COMP_FLAGS
"-O3"
"-m${HEXAGON_ARCH}"
)
string(REGEX REPLACE ";" " " EXTRA_COMP_FLAGS_STR "${EXTRA_COMP_FLAGS}")
- message(STATUS "EXTRA_COMP_FLAGS_STR: ${EXTRA_COMP_FLAGS_STR}")
set(CMAKE_C_FLAGS "${EXTRA_COMP_FLAGS_STR} ${CMAKE_C_FLAGS}")
set(CMAKE_CXX_FLAGS "${EXTRA_COMP_FLAGS_STR} ${CMAKE_CXX_FLAGS}")
set(EXTRA_LINK_FLAGS
- "-Wl,--no-threads"
- "-Wl,--wrap=malloc"
- "-Wl,--wrap=calloc"
- "-Wl,--wrap=free"
- "-Wl,--wrap=realloc"
- "-Wl,--wrap=memalign"
- "-Wl,--wrap=posix_memalign"
- "-Wl,--wrap=__stack_chk_fail"
+ "-Wl,--no-threads"
+ "-Wl,--wrap=malloc"
+ "-Wl,--wrap=calloc"
+ "-Wl,--wrap=free"
+ "-Wl,--wrap=realloc"
+ "-Wl,--wrap=memalign"
+ "-Wl,--wrap=posix_memalign"
+ "-Wl,--wrap=__stack_chk_fail"
)
string(REGEX REPLACE ";" " " EXTRA_LINK_FLAGS_STR "${EXTRA_LINK_FLAGS}")
# Extra linker flags for linking shared libraries.
set(CMAKE_SHARED_LINKER_FLAGS
- "${EXTRA_LINK_FLAGS_STR} ${CMAKE_SHARED_LINKER_FLAGS}")
+ "${EXTRA_LINK_FLAGS_STR} ${CMAKE_SHARED_LINKER_FLAGS}"
+ )
Review comment:
Setting global flags like this can be problematic if this project is
used by another. Can you set these flags on the specific targets they are
needed for. You can use `set_target_properties(libraryname PROPERTIES
LINK_FLAGS ${EXTRA_LINK_FLAGS_STR})`.
--
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]
