Separate host arch specific code to directories #75
Conversation
| @@ -78,6 +78,17 @@ include(${PROJECT_SOURCE_DIR}/cmake/dependencies.cmake) | |||
| add_library(umd_common_directories INTERFACE) | |||
| target_include_directories(umd_common_directories INTERFACE ${PROJECT_SOURCE_DIR} ${PROJECT_SOURCE_DIR}/device ${PROJECT_SOURCE_DIR}/third_party/fmt/include) | |||
|
|
|||
| if(CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64" OR CMAKE_SYSTEM_PROCESSOR MATCHES "i386") | |||
There was a problem hiding this comment.
Per offline discussion, include .cpp sources by host arch instead of headers, that way interface to metal will be the same.
The drawback, though, will be that the compiler won't be able to remove function call in that case. Wondering if that might even be a problem... According to github copilot, this shouldn't be a problem.
It could be a perf hit though.
In anycase, these should be in a separate utils repo. Started an issue here: #78
There was a problem hiding this comment.
These function cannot be inline anymore since the implementation will move to .cpp files. Are we ok with this?
If we want to keep it inline, we could just guard include headers with defines like
#if defined (__x86_64__)
#include "x86/driver_atomics.h"
#endif
// empty file for device/driver_atomics.h
and move the implementation to separate .h files based on the architecture. This way we don't need to change cmake. Next step could be to add dependency on host arch to cmake (or investigate if there already is some dependency) in order to completely remove device/driver_atomics.h
|
The conditional compilation for x86 vs arm vs riscv is fine as it's not something that the build system or user needs to worry about. I am not convinced that the additional complexity this introduces (multiple files, additional build logic) is worth the benefit. |
|
This requires some changes in the build system of Metal, which is not a priority at the moment, I will move the PR to draft just to not forget and we can change what is needed later on
OK, makes sense. If it somehow starts to blow up, we can come back and reconsider |
Fixes #56
driver_atomicsfiles separated into specific directories for each possible host arch that we want to support for nowTODO
device/arch. Do we need to move it elsewhere, for example roothost_archdirectory or something like that