[Parallel_libs-commits] [PATCH] D24368: [StreamExecutor] Make SE work with an in-tree LLVM build.

[Jason Henline via Parallel_libs-commits]

jhen added inline comments.

================
Comment at: streamexecutor/CMakeLists.txt:40
@@ -39,7 +39,3 @@
     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${LLVM_CXXFLAGS}")
 
     if(STREAM_EXECUTOR_UNIT_TESTS)
----------------
I added jprice's suggested three line fix here, and it got the standalone build working:

```
set(LLVM_CMAKE_PATH "${LLVM_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm")
list(APPEND CMAKE_MODULE_PATH "${LLVM_CMAKE_PATH}")
include(AddLLVM)
```

================
Comment at: streamexecutor/lib/CMakeLists.txt:2
@@ +1,3 @@
+macro(add_se_library name)
+  add_llvm_library(${name} ${ARGN})
+  set_target_properties(${name} PROPERTIES FOLDER "streamexecutor libraries")
----------------
jprice wrote:
> jlebar wrote:
> > jhen wrote:
> > > This breaks the standalone build because it doesn't recognize `add_llvm_library`.
> > Right.
> > 
> > I am not entirely hep to this jive, so I know I'm missing some stuff.  But lld uses add_llvm_library, and clang uses llvm_add_library (which would have the same problem?).
> > 
> > So my question is whether we can't do whatever they do, and be compatible with whatever types of builds they are compatible with?
> > 
> > If the standalone build uses llvm's headers, can it not also use its CMake files?
> > 
> > Same for add_unittest.
> Looking at the Clang CMakeLists.txt, I believe the three magic lines that do this for standalone builds are these:
> 
>     set(LLVM_CMAKE_PATH "${LLVM_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm")
>     list(APPEND CMAKE_MODULE_PATH "${LLVM_CMAKE_PATH}")
>     include(AddLLVM)
> 
> If I add these to this patch, I can successfully build SE both standalone and in-tree. Disclaimer: I'm no CMake expert.
> 
Those three lines work for me as well. Thanks jprice! I put in another comment to indicate exactly where I put those lines to make it work.

One downside from my standpoint is that I can't run `ninja test` to run the tests, I have to run the following two commands:

```
$ ninja unittests/StreamExecutorUnitTests
$ unittests/CoreTests/CoreTests
```

But we can figure that out in another patch (probably when we figure out how to support `check-streamexecutor` for the in-tree build), so I would say this is good to go in after those three lines are added.


https://reviews.llvm.org/D24368