This is an archive of the discontinued LLVM Phabricator instance.

[StreamExecutor] Make SE work with an in-tree LLVM build.
ClosedPublic

Authored by jlebar on Sep 8 2016, 5:04 PM.

Details

Summary

With these changes, we can put parallel-libs within llvm/projects and
build as normal.

This is kind of the minimal change I could figure out how to make while
still making us compatible with llvm's build system. Some things I'm
not thrilled about include:

  • The creation of a CoreTests directory (the macros really seemed to want this)
  • Pulling SimpleHostPlatformDevice.h into CoreTests. It seems to me this should live inside unittests/include, or maybe tests/include, but I didn't want to make that change in this patch.

One important piece of work that remains to be done is to make

$ ninja check-streamexecutor

run all the tests. Right now the only way I've figured out to run the
tests is

$ ninja projects/parallel-libs/streamexecutor/unittests/StreamExecutorUnitTests
$ projects/parallel-libs/streamexecutor/unittests/CoreTests/CoreTests

Diff Detail

Repository
rL LLVM

Event Timeline

jlebar updated this revision to Diff 70768.Sep 8 2016, 5:04 PM
jlebar retitled this revision from to [StreamExecutor] Make SE work with an in-tree LLVM build..
jlebar updated this object.
jlebar added a reviewer: jhen.
jlebar added subscribers: jprice, parallel_libs-commits.
jhen edited edge metadata.Sep 8 2016, 6:11 PM
streamexecutor/lib/CMakeLists.txt
2 ↗(On Diff #70768)

This breaks the standalone build because it doesn't recognize add_llvm_library.

streamexecutor/unittests/CMakeLists.txt
5 ↗(On Diff #70768)

I think add_unittest is also defined by the LLVM cmake files, so the standalone build will break here too.

jlebar added inline comments.Sep 8 2016, 6:17 PM
streamexecutor/lib/CMakeLists.txt
2 ↗(On Diff #70768)

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.

jprice added inline comments.Sep 9 2016, 9:27 AM
streamexecutor/lib/CMakeLists.txt
2 ↗(On Diff #70768)

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.

jhen added inline comments.Sep 9 2016, 10:35 AM
streamexecutor/CMakeLists.txt
40 ↗(On Diff #70768)

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)
streamexecutor/lib/CMakeLists.txt
2 ↗(On Diff #70768)

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.

jhen accepted this revision.Sep 9 2016, 10:36 AM
jhen edited edge metadata.
This revision is now accepted and ready to land.Sep 9 2016, 10:36 AM
jlebar marked 6 inline comments as done.Sep 9 2016, 2:08 PM

Thanks for all the help, folks. Submitting this now.

This revision was automatically updated to reflect the committed changes.
jprice added a comment.Sep 9 2016, 4:06 PM

I've just noticed that this patch split the Utils target into a separate installed library (libutils.a), which breaks things building against SE using streamexecutor-config.

Was this intentional? If it was, I guess it needs to be added to streamexecutor-config, but maybe also renamed to something less generic (e.g. libstreamexecutor-utils.a)?

jlebar added a comment.Sep 9 2016, 4:26 PM

I've just noticed that this patch split the Utils target into a separate installed library (libutils.a), which breaks things building against SE using streamexecutor-config.

Was this intentional? If it was, I guess it needs to be added to streamexecutor-config, but maybe also renamed to something less generic (e.g. libstreamexecutor-utils.a)?

Hm. My intent was for the utils to be statically linked into the main static library. But clearly that didn't happen.

Jason, what if we just merged everything into one big directory (for now)?

jhen added a comment.Sep 9 2016, 4:27 PM

I've just noticed that this patch split the Utils target into a separate installed library (libutils.a), which breaks things building against SE using streamexecutor-config.

Was this intentional? If it was, I guess it needs to be added to streamexecutor-config, but maybe also renamed to something less generic (e.g. libstreamexecutor-utils.a)?

Thanks for catching that! We could fix it in the make files, but instead I'll just put up a patch to get rid of Utils entirely. The error stuff should just be with the other stuff, and I blame myself for not fixing it up until now.

parallel-libs/trunk/streamexecutor/lib/unittests/SimpleHostPlatformDevice.h