[Examples] Add add_llvm_example_library macro and use it for IR example.

[fhahn]

Summary

We need to make sure to set the right destination for the ExamplesIRTransforms
library target and respect LLVM_BUILD_EXAMPLES. This patch adds a
add_llvm_example_library macro similar to the add_llvm_example one,
which should set the destination for the install target properly and
exclude it from the ALL target if LLVM_BUILD_EXAMPLES=Off.

It should fix a problem where using LLVMExports.cmake fails, because
it expects libExampleIRTransforms.a to be installed.

[andwar]

llvm/examples/IRTransforms/CMakeLists.txt

7

[nit] Could you add a comment explaining why this is needed? Currently it says what is being done, but that's clear from the code. This could useful to our future selves.

[fhahn]

Update the patch to add a dedicated add_llvm_example_library macro, similar to add_llvm_example, which takes care of respecting LLVM_BUILD_EXAMPLES and setting the correct destinations.

[fhahn]

llvm/examples/IRTransforms/CMakeLists.txt

7

Thanks for taking a look. I move the logic into a macro similar to add_llvm_example, which now takes care of respecting LLVM_BUILD_EXAMPLES and more.

[compnerd]

I think that rather than adding more wrapping macros, we should fix the structure. We can mark the examples directory from ALL at the add_subdirectory level and only add the subdirectory when examples are enabled. This avoids the need for the new macro. I think that we want to move towards a more standard cmake build rather than a more specialized one.

[fhahn]

In D70590#1757902, @compnerd wrote:

I think that rather than adding more wrapping macros, we should fix the structure. We can mark the examples directory from ALL at the add_subdirectory level and only add the subdirectory when examples are enabled. This avoids the need for the new macro. I think that we want to move towards a more standard cmake build rather than a more specialized one.

I think sticking to more standard cmake is great. My Cmake is a bit rusty, are you suggesting to just not add the examples directory (guarding add_subdirectory with LLVM_BUILD_EXAMPLES)?

I think that would change the behavior slightly, because currently IIUC we always add the example targets, we just not build them by default and also update the target directory for installing them and it would be valuable to keep the current behavior.

[w88]

In D70590#1758209, @fhahn wrote:

In D70590#1757902, @compnerd wrote:

I think that rather than adding more wrapping macros, we should fix the structure. We can mark the examples directory from ALL at the add_subdirectory level and only add the subdirectory when examples are enabled. This avoids the need for the new macro. I think that we want to move towards a more standard cmake build rather than a more specialized one.

I think sticking to more standard cmake is great. My Cmake is a bit rusty, are you suggesting to just not add the examples directory (guarding add_subdirectory with LLVM_BUILD_EXAMPLES)?

I think that would change the behavior slightly, because currently IIUC we always add the example targets, we just not build them by default and also update the target directory for installing them and it would be valuable to keep the current behavior.

[lebedev.ri]

Would it please be possible to have some progress on the problem?
Either this needs to land, or if it is not in an acceptable form then D69416 needs to be reverted till green.

[fhahn]

Reverted as 19fd8925a4afe6efd248688cce06aceff50efe0c so we do not need to rush this fix.

[fhahn]

@compnerd ping. Do you have any thoughts about my earlier response (https://reviews.llvm.org/D70590#1758209) with respect to just use add_subdirectory to control the inclusion of the examples? IIUC that would change the current behavior (example targets are added even with LLVM_BUILD_EXAMPLES=Off, they are just excluded from the ALL target and the installed stuff)

[fhahn]

ping @compnerd

It would be great if you could take a look to see if my previous response makes sense to you :)

[compnerd]

Hmm, I see, yes, that would change the behavior where we would not create the targets for it. In that case, using EXCLUDE_FROM_ALL is fine, but set it as a target property rather than passing global variables like this. But, even better would be to understand if we even need this.

Really, the install-distribution target allows the installed image to contain specific bits, and the distribution target lets you build the minimal set. Having the target being built always when you build *everything* (ALL) is the preferable option.

[fhahn]

In D70590#1800672, @compnerd wrote:

Hmm, I see, yes, that would change the behavior where we would not create the targets for it. In that case, using EXCLUDE_FROM_ALL is fine, but set it as a target property rather than passing global variables like this. But, even better would be to understand if we even need this.

Really, the install-distribution target allows the installed image to contain specific bits, and the distribution target lets you build the minimal set. Having the target being built always when you build *everything* (ALL) is the preferable option.

Thanks for taking a look. The original commit was reverted, so I'll re-commit the original change with the EXCLUDE_FROM_ALL fix. I'll check how to not pass it as global variable.

[andwar]

@fhahn Since https://reviews.llvm.org/D61446 has finally landed, what about revisiting the idea of using that instead? Just a thought.

[fhahn]

In D70590#1803584, @andwar wrote:

@fhahn Since https://reviews.llvm.org/D61446 has finally landed, what about revisiting the idea of using that instead? Just a thought.

Sure, I just re-landed the original patch, but using the new infrastructure is certainly something we should do now.

llvm/cmake/modules/AddLLVM.cmake

macro(add_llvm_example name)
   endif()
   add_llvm_executable(${name} ${ARGN})
   if( LLVM_BUILD_EXAMPLES )
     install(TARGETS ${name} RUNTIME DESTINATION examples)
   endif()
   set_target_properties(${name} PROPERTIES FOLDER "Examples")
 endmacro(add_llvm_example name)
 
+macro(add_llvm_example_library name)
+  if( NOT LLVM_BUILD_EXAMPLES )
+    set(EXCLUDE_FROM_ALL ON)
+  endif()
+  add_llvm_library (${name} ${ARGN})
+  if( LLVM_BUILD_EXAMPLES )
+    install(TARGETS ${name} RUNTIME DESTINATION examples)
+  endif()
+  set_target_properties(${name} PROPERTIES FOLDER "Examples")
+endmacro(add_llvm_example_library name)
+
 # This is a macro that is used to create targets for executables that are needed
 # for development, but that are not intended to be installed by default.
 macro(add_llvm_utility name)
   if ( NOT LLVM_BUILD_UTILS )
     set(EXCLUDE_FROM_ALL ON)
   endif()
 
   add_llvm_executable(${name} DISABLE_LLVM_LINK_LLVM_DYLIB ${ARGN})

llvm/examples/IRTransforms/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Analysis
   Core
   Support
   )
 
-add_llvm_library(ExampleIRTransforms
+add_llvm_example_library(ExampleIRTransforms
   InitializePasses.cpp
   SimplifyCFG.cpp
 
   ADDITIONAL_HEADER_DIRS
 
   DEPENDS
   intrinsics_gen
   )