leezu commented on a change in pull request #17559: URL: https://github.com/apache/incubator-mxnet/pull/17559#discussion_r479539243
########## 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: > This is common practice: I think the MXNet source is not always a good reference for best practices :) > Were I to do that, it would unnecessarily pollute the include paths of software that references the file by its path in the source/submodule tree. Having files hardcode the direct path in the submodule tree limits the use of intgemm. For example, consider users that adopt conan.io or linux distributions that prefer to package intgemm separately from MXNet. > And it would potentially introduce header name conflicts. It's common practice to put the public header files into `include/intgemm` instead of the top level directory. This will avoid any name conflicts. ---------------------------------------------------------------- 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]
