kpuatamazon commented on a change in pull request #17559: URL: https://github.com/apache/incubator-mxnet/pull/17559#discussion_r479475733
########## File path: CMakeLists.txt ########## @@ -278,6 +283,25 @@ if(USE_MKLDNN) set_target_properties(dnnl PROPERTIES CXX_CLANG_TIDY "") # don't lint 3rdparty dependency endif() +if(USE_INTGEMM) + message(STATUS "Using intgemm") + include(FetchContent) + FetchContent_Declare( + intgemm + GIT_REPOSITORY https://github.com/kpu/intgemm.git + GIT_TAG 0f05c3ebd037eacdf8cff165736fea2b0d125023 + ) + FetchContent_GetProperties(intgemm) + if(NOT intgemm_POPULATED) + FetchContent_Populate(intgemm) + add_subdirectory(${intgemm_SOURCE_DIR} ${intgemm_BINARY_DIR} EXCLUDE_FROM_ALL) + endif() + include_directories(${intgemm_SOURCE_DIR}) + #intgemm generates a config header based on AVX512 support in the compiler. + include_directories(${intgemm_BINARY_DIR}) Review comment: I've removed `include_directories(${intgemm_BINARY_DIR})`. However, `include_directories(${intgemm_SOURCE_DIR})` remains. intgemm itself has entirely relative header paths and users do not need to `-I` the source directory to use it, so it would be incorrect to add that to the library. The only reason is that you asked for `FetchContent` which implies the code moves around which implies that MXNet needs some what to find the header files to begin with. Were this to convert back to a submodule, the path to intgemm would be constant and `include_directories(${intgemm_SOURCE_DIR})` could be removed. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
