WIP: Fix MacOS build CI #882
Conversation
|
Now that even the compiler-accelerated CPU backend has been reported working in #883, maybe we could test that in CI on Mac too? |
.github/workflows/macos.yml
Outdated
| -DOpenMP_CXX_LIB_NAMES=libomp \ | ||
| -DOpenMP_CXX_FLAGS="-Xclang -fopenmp" \ | ||
| -DOpenMP_CXX_INCLUDE_DIR=/usr/local/Cellar/libomp/15.0.5/include \ | ||
| -DWITH_ACCELERATED_CPU=ON \ |
There was a problem hiding this comment.
If we enable accelerated CPU, wouldn't we also need to install LLVM first? Or does it also work with Apple clang?
There was a problem hiding this comment.
(And are there even Mac packages for LLVM? Compiling it in CI could take forever..)
There was a problem hiding this comment.
Guess it should work, but will be a similar situation as with ROCm... versioning is quite different for Apple clang...
There was a problem hiding this comment.
Meaning it probably won't work then :P I guess then the question is whether we want to support Apple clang with accelerated cpu to the extent that we test it in CI..
There was a problem hiding this comment.
yeah, that's your decision, I guess.. 😇
at least brew should cover your upstream LLVM needs :) brew install llvm
There was a problem hiding this comment.
@normallytangent note the difference: OMP_CXX_FLAGS vs OpenMP_CXX_FLAGS where the first one is consumed when configuring the hipSYCL build (not the tests), setting the default for syclcc which is then automatically used for building the tests.
OpenMP_CXX_FLAGS are unused by hipSYCL.
There was a problem hiding this comment.
Thank you so much! I was so confused why the flags used during building hipsycl were not being used by when building the tests. If I understand correctly, I need to give both the OMP_CXX_FLAGS and OpenMP_CXX_FLAGS. One for cmake to find the libomp package, and the latter for configuring hipSYCL
There was a problem hiding this comment.
Looking at https://github.com/illuhad/hipSYCL/blob/develop/src/runtime/CMakeLists.txt#L181, I think hipSYCL indeed uses the FindOpenMP module? So yeah, those OpenMP_ variables will likely have an effect on whether or not that find_package is happy. Might be that setting OpenMP_CXX_INCLUDE_DIR would be what you'd want to do then? (cf. https://cmake.org/cmake/help/latest/module/FindOpenMP.html, https://github.com/Kitware/CMake/blob/master/Modules/FindOpenMP.cmake)
On a slightly related note (seeing your last commit): I would guess it might make sense to add OMP_CXX_FLAGS_EXTRA (and similarly for *_LINK_* and the other backends) options to hipSYCL's main CMakeLists.txt that are just appended to their respective OMP_CXX_FLAGS? (so that one doesn't have to override the full commandline, e.g. messing up the boost include paths)
There was a problem hiding this comment.
@fodinabor I had originally added OpenMP_CXX_INCLUDE_DIR. Later I removed it, when I started passing the path via -I flag to OpenMP_CXX_FLAGS. This commit had built successfully. I've since been unable to reproduce it when I reduce the number of CMAKE options. Another solution probably would be to export LDFLAGS and CPPFLAGS for libomp but I didn't really try it on the CI.
Thanks for the idea about how to append those flags, I was actually finding a way to do just this, but from the user's point of view.
There was a problem hiding this comment.
Mhh.. how about providing OMP_ROOT as such -DOpenMP_ROOT=$OMP_ROOT to the hipSYCL build? (alternatively -DCMAKE_PREFIX_PATH=$OMP_ROOT might also work...?)
Maybe that still needs -DCMAKE_CXX_FLAGS=-I$OMP_ROOT/include for the tests though..?
Is that maybe sufficient?
Setting OMP_CXX_FLAGS as currently is, seems brittle (with the boost issue..), but with the _EXTRA thing you could do -DOMP_CXX_FLAGS_EXTRA=-I$OMP_ROOT/include.
Co-authored-by: fodinabor <5982050+fodinabor@users.noreply.github.com>
Co-authored-by: fodinabor <5982050+fodinabor@users.noreply.github.com>
Yes, it is ready to merge. Should I merge it? If yes, then should I 'create a merge commit'? |
|
I merge :) |
No description provided.