Page MenuHomePhabricator

[Flang] add some cmake code to allow for out-of-tree building of MLIR and LLVM
ClosedPublic

Authored by schweitz on Jan 8 2020, 2:53 PM.

Details

Summary

The flang(f18) developers desire a way to build llvm and mlir separately for installation and then build the flang front-end out of tree. This patch adds some cmake infrastructure to allow that development environment to behave correctly.

Diff Detail

Event Timeline

schweitz created this revision.Jan 8 2020, 2:53 PM
Herald added a project: Restricted Project. · View Herald Transcript
schweitz retitled this revision from add some cmake code to allow for out-of-tree building of MLIR and LLVM for flang to [Flang] add some cmake code to allow for out-of-tree building of MLIR and LLVM.Jan 9 2020, 10:51 AM
schweitz edited the summary of this revision. (Show Details)
jpienaar marked an inline comment as done.Jan 14 2020, 8:16 AM

Nice, seems to match what clang does here too so that's good to keep it in sync

mlir/CMakeLists.txt
8–9

Cant this be removed now?

39–40

Can this be removed now?

mlir/cmake/modules/CMakeLists.txt
2–4

I was going to comment "What does clang do?" and then I saw they have this exact same comment ;-)

29–30

Nit: could you indent these to show the nesting?

36

Same re indent

44–47

Leftover from testing?

52

Could you add a comment for this section?

schweitz marked 4 inline comments as done.Jan 14 2020, 9:48 AM
schweitz added inline comments.
mlir/CMakeLists.txt
8–9

Possibly. We can give it a try.

mlir/cmake/modules/CMakeLists.txt
29–30

Sure. :)

44–47

This was leftover from cobbling things together from other LLVM projects. Other projects seem to setup this XXXConfig.cmake file, but I found it wasn't needed here.

We can remove these commented out lines, if you prefer. I left them with the chance someone who knew CMake better had good reasons to build out an MLIRConfig.cmake rule.

52

I can give it a shot.

I think you may have forgotten to upload the changed diff :) In general I think this looks good, I'd like to remove the duplicate functions but beyond that this should be good.

mlir/cmake/modules/CMakeLists.txt
44–47

Lets remove them, instead thanks.

schweitz updated this revision to Diff 238573.Jan 16 2020, 12:21 PM
schweitz marked an inline comment as done.
schweitz added inline comments.
mlir/cmake/modules/CMakeLists.txt
29–30

Actually, this looks like it is in column-0 because it is quoted. I'm going to leave it.

schweitz marked 3 inline comments as done and 2 inline comments as done.Jan 16 2020, 12:22 PM
jpienaar accepted this revision.Jan 17 2020, 5:23 PM

Looks good thanks

This revision is now accepted and ready to land.Jan 17 2020, 5:23 PM

Do you need someone to land this or do you have commit access now @schweitz ?

Do you need someone to land this or do you have commit access now @schweitz ?

I will need someone to land it. I don't have commit access. Thanks in advance.

Can you rebase the patch? It does not apply right now

schweitz updated this revision to Diff 239445.Tue, Jan 21, 3:33 PM

Do you have the ability to use arc by the way? It enables the CI testing to build/test revision here

This revision was automatically updated to reflect the committed changes.
awson added a subscriber: awson.Wed, Jan 22, 10:26 PM

This patch breaks Windows MSVC build, former correct ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib/${LIB}.lib now suddenly becomes wrong ${LIB}.

This patch breaks Windows MSVC build, former correct ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib/${LIB}.lib now suddenly becomes wrong ${LIB}.

I'll revert this part to what it was given https://bugs.llvm.org/show_bug.cgi?id=44660