Page MenuHomePhabricator

[MLIR] [CMake] Support building MLIR standalone
ClosedPublic

Authored by mgorny on Aug 6 2020, 12:04 PM.

Details

Summary

Add the necessary bits to CMakeLists to make it possible to configure
MLIR against installed LLVM, and build it with minimal need for LLVM
source tree. The latter is only necessary to run unittests, and if it
is missing then unittests are skipped with a warning.

This change includes the necessary changes to tests, in particular
adding some missing substitutions and defining missing variables
for lit.site.cfg.py substitution.

Diff Detail

Event Timeline

mgorny created this revision.Aug 6 2020, 12:04 PM
mgorny requested review of this revision.Aug 6 2020, 12:04 PM
mlir/CMakeLists.txt
39–48

This seems like exactly the kindof boilerplate that 'every' project needs. I'd like to work refactoring this so that projects have more commonality in how they integrate with CMAKE. (I'll point you to a review along these lines)

54–57

This exists check seems awkward. I'd expect that this should actually be a configuration option?

mlir/lib/Conversion/GPUToNVVM/CMakeLists.txt
4 ↗(On Diff #283695)

I don't understand why these include directories should be different when MLIR is built out-of-tree.

mgorny added inline comments.Aug 6 2020, 1:24 PM
mlir/CMakeLists.txt
39–48

I don't have a strong opinion on this. However, some code would have to repeated to get the 'common' stuff in anyway ;-).

54–57

I don't know if it's something worth configuring. The rough idea is that you either build with complete LLVM source tree available (and I make here a rough default for monorepo) and then gtest is available, or you don't and then it's not.

mlir/lib/Conversion/GPUToNVVM/CMakeLists.txt
4 ↗(On Diff #283695)

Me neither. However, for some reason they're missing there and build is failing to find generated includes. I'm afraid I don't know how to debug this.

jpienaar added inline comments.Aug 9 2020, 4:32 PM
mlir/CMakeLists.txt
1–27

Could we have this line at the top still?

54–57

Could TARGET gtest not be used here too?

mlir/lib/Conversion/GPUToNVVM/CMakeLists.txt
4 ↗(On Diff #283695)

What errors do you see?

mgorny marked an inline comment as done.Aug 9 2020, 7:30 PM
mgorny added inline comments.
mlir/CMakeLists.txt
1–27

Sure, done that.

54–57

There's no TARGET until you add_subdirectory.

mlir/lib/Conversion/GPUToNVVM/CMakeLists.txt
4 ↗(On Diff #283695)

It doesn't find generated .inc files (i.e. -I${CMAKE_CURRENT_BINARY_DIR} is clearly missing for some reason).

mgorny updated this revision to Diff 284248.Aug 9 2020, 7:30 PM
mgorny marked an inline comment as done.

Moved comment back to the top.

lattner resigned from this revision.Aug 9 2020, 9:03 PM
mlir/lib/Conversion/GPUToNVVM/CMakeLists.txt
4 ↗(On Diff #283695)

LLVM does: CMakeLists.txt:include_directories( ${LLVM_INCLUDE_DIR} ${LLVM_MAIN_INCLUDE_DIR})
which MLIR inherits in an in-tree build

I think the correct thing to do is to have something similar:
include_directories( ${MLIR_INCLUDE_DIR} )

This should avoid including this in all of the dialects.

ftynse resigned from this revision.Sep 29 2020, 6:02 AM
mravishankar resigned from this revision.Oct 26 2020, 10:32 AM
silvas resigned from this revision.Oct 27 2020, 4:37 PM
isuruf added a subscriber: isuruf.Jan 29 2021, 5:14 PM

Is there any interest in this? If so, I can rebase this on top of master and send it.

My interest here is to package MLIR for a package manager and having standalone builds makes it much easier to separately package MLIR and LLVM.

If you rebase, I can try to help the review if @stephenneuendorffer does not have cycles available.

isuruf updated this revision to Diff 320316.Jan 30 2021, 11:01 AM
isuruf edited the summary of this revision. (Show Details)

Rebased on top of master and fixes for new directories

stephenneuendorffer requested changes to this revision.Jan 30 2021, 12:12 PM
stephenneuendorffer added inline comments.
mlir/lib/Conversion/GPUToNVVM/CMakeLists.txt
4 ↗(On Diff #283695)

I still think there should be a better solution than doing this in all dialects.... maybe add it to mlir_tablegen()?

This revision now requires changes to proceed.Jan 30 2021, 12:12 PM
isuruf updated this revision to Diff 320323.Jan 30 2021, 1:12 PM

Move adding CMAKE_CURRENT_BINARY_DIR to tablegen function

Looks good to me. Thanks for doing this!

This revision is now accepted and ready to land.Jan 31 2021, 10:06 AM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Feb 3 2021, 2:06 PM
mlir/CMakeLists.txt
140

This logic does not work for me: I suddenly get "gtest not found, unittests will not be available" in my regular build.

isuruf added inline comments.Feb 3 2021, 2:34 PM
mlir/CMakeLists.txt
140

Thanks for letting me know. Fix in https://reviews.llvm.org/D95978

mehdi_amini added inline comments.Feb 10 2021, 5:19 PM
mlir/test/CMakeLists.txt
60

Since this change, unit-tests are not built with ninja check-mlir anymore. I tried a fix in http://github.com/llvm/llvm-project/commit/09cfec62432993abde7829a88ce4793d8805ce4b ; please take a look!