[cmake] Add INTERFACE_INCLUDE_DIRECTORIES to LLVM and MLIR.

[stellaraccident]

Summary

We've long needed to do this and it really helps out of tree projects since it eliminates the need to set up include paths when taking a source dep on LLVM or MLIR libraries (every external project I work on has some ~dozen lines of include path hackery to make it work, and this eliminates it).

In the future, as the project structures evolve, we could bootstrap the public include paths off of directory properties if that helps (I've seen repo restructuring proposals that may need this flexibility). In any case, we should not require clients of libraries to know those details.

This will let me roll back some hacks from out of tree projects, which will leave a couple of things (likely tablegen related), which we can also look to fix.

This patch has no effect on installed deps (that is what the $<BUILD_INTERFACE:> expression does -- scopes it to source deps in the build system). We may also want to use the $<INSTALL_INTERFACE:> expression to make installed builds automatic.

[marbre]

Thanks for working on this Stella! Untangling the functions/marcos is a bit hard. Thus, I am mainly commenting to make sure I fully understand the patch :)

llvm/cmake/modules/AddLLVM.cmake

821

With your change to llvm_add_library() the generated objlib can propagate "public" includes, as PUBLIC in setting INCLUDE_DIRECTORIES and INTERFACE_INCLUDE_DIRECTORIES (which looks good to me 👍).

Here, the public includes are added after the calling llvm_add_library(). A rapidly prototyped test script lets me assume that the order might important. Should the public includes be added before calling llvm_add_library() to mirror those?

[stellaraccident]

llvm/cmake/modules/AddLLVM.cmake

821

Here, the public includes are added after the calling llvm_add_library(). A rapidly prototyped test script lets me assume that the order might important. Should the public includes be added before calling llvm_add_library() to mirror those?

Can you say more -- I'm not quite sure I'm following? Iiuc, the existing include mechanism depends on bootstrapping from the directory INCLUDE_DIRECTORIES property (which is a vestige of legacy interop). There is not such a mechanism for INTERFACE_INCLUDE_DIRECTORIES, so it does need to be after library creation. But I'm not seeing a more subtle order dependence (not saying there isn't one, just not seeing it).

[marbre]

Please excuse if my previous reply was confusing. I checked more carefully and think these changes are good to go.

llvm/cmake/modules/AddLLVM.cmake

501

With this, the interface include directories (usage-requirements) of the object libraries are mirrored. I would have assumed that mirroring the private include directories (build-requirements) might be sufficient, but seems I am wrong.

821

I think the generator-expressions actually confused me a little. In llvm_add_library(), the target ${name} is created after ${obj_name}. Anyway, build-requirements and usage-requirements are propagated from ${name} to ${obj_name}. Since generator-expressions aren't resolved at runtime but at build time, this should work out.
Here, we have an generator-expression as well, but target_include_directories() of course needs an existing target. Unfortunately, I missed that the add_library() command for ${name} is called inside llvm_add_library(). Only looking at the properties one could have moved this up, but not from the target perspective. So this is the way to do it:

Setting target_include_directories here should be perfectly fine. Even though includes are still set via include_directories( ${LLVM_INCLUDE_DIR} ${LLVM_MAIN_INCLUDE_DIR}) in the llvm/CMakeLists.txt, setting the includes for the specific target is the preferred way and hopefully brings us closer to a state where we can drop the include_directories command. Thanks Stella!

[stellaraccident]

llvm/cmake/modules/AddLLVM.cmake

501

Ok, I think you may be right because of "The INTERFACE, PUBLIC and PRIVATE keywords are required to specify the scope of the following arguments. PRIVATE and PUBLIC items will populate the INCLUDE_DIRECTORIES property of <target>. PUBLIC and INTERFACE items will populate the INTERFACE_INCLUDE_DIRECTORIES property of <target>. The following arguments specify include directories."

So a target_include_directories(Foo PUBLIC dir/) will populate both the INCLUDE_DIRECTORIES and INTERFACE_INCLUDE_DIRECTORIES. By that logic, this should not be necessary.

[stellaraccident]

llvm/cmake/modules/AddLLVM.cmake

821

I think the generator-expressions actually confused me a little. In llvm_add_library(), the target ${name} is created after ${obj_name}. Anyway, build-requirements and usage-requirements are propagated from ${name} to ${obj_name}. Since generator-expressions aren't resolved at runtime but at build time, this should work out.

CMake is confusion if you think too much about it :) These generator expressions are just strings. They only get evaluated at configure time once all targets have been instantiated. The order that the strings are created makes no difference.

[marbre]

llvm/cmake/modules/AddLLVM.cmake

501

Yes, PRIVATE sets INCLUDE_DIRECTORIES, INTERFACE sets INTERFACE_INCLUDE_DIRECTORIES and with PUBLIC both are set. This can be easily checked with the CMake code attached.

Anyway, do we really need to set INTERFACE_INCLUDE_DIRECTORIES for object libraries?

cmake_minimum_required(VERSION 3.13.4)

project(cmake_test LANGUAGES C)
set(TEST_INCLUDE_DIR ${PROJECT_SOURCE_DIR}/include)

file(WRITE dummy.c)
add_library(test_private "dummy.c")
add_library(test_interface "dummy.c")
add_library(test_public "dummy.c")

macro(print_includes name)
  message(STATUS "Target \"${name}\"")
  get_target_property(_INCLUDES ${name} INCLUDE_DIRECTORIES)
  message(STATUS "  INCLUDE_DIRECTORIES:")
  foreach(dir ${_INCLUDES})
    message(STATUS "    ${dir}")
  endforeach()

  get_target_property(_INCLUDES ${name} INTERFACE_INCLUDE_DIRECTORIES)
  message(STATUS "  INTERFACE_INCLUDE_DIRECTORIES:")
  foreach(dir ${_INCLUDES})
    message(STATUS "    ${dir}")
  endforeach()
endmacro()

target_include_directories(test_private PRIVATE ${TEST_INCLUDE_DIR})
print_includes(test_private)
target_include_directories(test_interface INTERFACE ${TEST_INCLUDE_DIR})
print_includes(test_interface)
target_include_directories(test_public PUBLIC ${TEST_INCLUDE_DIR})
print_includes(test_public)

[mehdi_amini]

llvm/cmake/modules/AddLLVM.cmake

501

Anyway, do we really need to set INTERFACE_INCLUDE_DIRECTORIES for object libraries?

What make an object library different?
If another library depends on an object library they would need to have the include for its public headers right?

[marbre]

llvm/cmake/modules/AddLLVM.cmake

501

I had the impression that object libraries within llvm_add_library are only used when building shared and static libs. In that case, the object libraries are passed as an argument to nested llvm_add_library calls that create the static and shared libraries. For these targets the includes are already set and the object libraries only mirror those.

Anyway, I just saw that the object libraries are required in AddMLIR.cmake, so yeah, probably also the public includes are needed there somewhere. So please go ahead with landing this :)

[stellaraccident]

FYI - Some intervening changes rendered this patch at least partially invalid and I never parsed back through and redid it. Consider it for inspirational value if anyone wants to pick it up.

llvm/cmake/modules/AddLLVM.cmake

if(CMAKE_GENERATOR STREQUAL "Xcode")
       file(WRITE ${DUMMY_FILE} "// This file intentionally empty\n")
       set_property(SOURCE ${DUMMY_FILE} APPEND_STRING PROPERTY COMPILE_FLAGS "-Wno-empty-translation-unit")
     endif()
     set(ALL_FILES "$<TARGET_OBJECTS:${obj_name}>" ${DUMMY_FILE})
 
     # Do add_dependencies(obj) later due to CMake issue 14747.
     list(APPEND objlibs ${obj_name})
 
-    # Bring in the target include directories from our original target.
-    target_include_directories(${obj_name} PRIVATE $<TARGET_PROPERTY:${name},INCLUDE_DIRECTORIES>)
-
-    set_target_properties(${obj_name} PROPERTIES FOLDER "Object Libraries")
+    # Mirror the private and interface include directories from the original
+    # target.
+    set_target_properties(${obj_name} PROPERTIES
+      INCLUDE_DIRECTORIES "$<TARGET_PROPERTY:${name},INCLUDE_DIRECTORIES>"
+      INTERFACE_INCLUDE_DIRECTORIES "$<TARGET_PROPERTY:${name},INTERFACE_INCLUDE_DIRECTORIES>"
+      FOLDER "Object Libraries"
+    )
     if(ARG_DEPENDS)
       add_dependencies(${obj_name} ${ARG_DEPENDS})
     endif()
     # Treat link libraries like PUBLIC dependencies.  LINK_LIBS might
     # result in generating header files.  Add a dependendency so that
     # the generated header is created before this object library.
     if(ARG_LINK_LIBS)
       cmake_parse_arguments(LINK_LIBS_ARG
macro(add_llvm_library name)
   if(ARG_MODULE)
     llvm_add_library(${name} MODULE ${ARG_UNPARSED_ARGUMENTS})
   elseif( BUILD_SHARED_LIBS OR ARG_SHARED )
     llvm_add_library(${name} SHARED ${ARG_UNPARSED_ARGUMENTS})
   else()
     llvm_add_library(${name} ${ARG_UNPARSED_ARGUMENTS})
   endif()
 
+  target_include_directories(${name} PUBLIC
+    $<BUILD_INTERFACE:${LLVM_MAIN_INCLUDE_DIR}>
+    $<BUILD_INTERFACE:${LLVM_INCLUDE_DIR}>
+  )
+
   # Libraries that are meant to only be exposed via the build tree only are
   # never installed and are only exported as a target in the special build tree
   # config file.
   if (NOT ARG_BUILDTREE_ONLY AND NOT ARG_MODULE)
     set_property( GLOBAL APPEND PROPERTY LLVM_LIBS ${name} )
     set(in_llvm_libs YES)
   endif()
 
add_lit_target(${target} ${comment}
     DEPENDS ${ARG_DEPENDS}
     ARGS ${ARG_ARGS}
     )
 endfunction()
 
 function(add_lit_testsuites project directory)
   if (NOT LLVM_ENABLE_IDE)
     cmake_parse_arguments(ARG "EXCLUDE_FROM_CHECK_ALL" "FOLDER" "PARAMS;DEPENDS;ARGS" ${ARGN})
-    
     if (NOT ARG_FOLDER)
       set(ARG_FOLDER "Test Subdirectories")
     endif()
 
     # Search recursively for test directories by assuming anything not
     # in a directory called Inputs contains tests.
     file(GLOB_RECURSE to_process LIST_DIRECTORIES true ${directory}/*)
     foreach(lit_suite ${to_process})

mlir/cmake/modules/AddMLIR.cmake

function(add_mlir_library name)
   get_property(llvm_component_libs GLOBAL PROPERTY LLVM_COMPONENT_LIBS)
   foreach(lib ${ARG_LINK_LIBS})
     if(${lib} IN_LIST llvm_component_libs)
       message(SEND_ERROR "${name} specifies LINK_LIBS ${lib}, but LINK_LIBS cannot be used for LLVM libraries.  Please use LINK_COMPONENTS instead.")
     endif()
   endforeach()
 
   list(APPEND ARG_DEPENDS mlir-generic-headers)
-  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs} DEPENDS ${ARG_DEPENDS} LINK_COMPONENTS ${ARG_LINK_COMPONENTS} LINK_LIBS ${ARG_LINK_LIBS})
+  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs}
+      DEPENDS ${ARG_DEPENDS}
+      LINK_COMPONENTS ${ARG_LINK_COMPONENTS}
+      LINK_LIBS ${ARG_LINK_LIBS})
+
+  target_include_directories(${name} PUBLIC
+    $<BUILD_INTERFACE:${MLIR_SOURCE_DIR}/include>
+    $<BUILD_INTERFACE:${MLIR_INCLUDE_DIR}>
+  )
 
   if(TARGET ${name})
     target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})
     if(NOT ARG_DISABLE_INSTALL)
       add_mlir_library_install(${name})
     endif()
   else()
     # Add empty "phony" target