[MLIR] Fix build with make

[nikic]

Summary

https://reviews.llvm.org/D124075 causes MLIR to no longer build when using make rather than ninja, due to a tablegen-generated header being used before it is created.

It seems that this is related to the use of LLVM_ENABLE_OBJLIB when using add_tablgen with a non-Ninja/Xcode generator. In that case an intermediate obj target is generated.

This patch fixes the issue by a) declaring dependencies in add_tablegen for mlir-pdll and b) making sure those dependencies are added to the obj target.

[serge-sans-paille]

Looks good to me, but I'd love an extra ack from someone in the mlir project. @mehdi_amini ?

[marbre]

I am currently out-of-office and don‘t have any access to my build machines. Therefore, I cannot test and confirm that the patch doesn‘t have unintended side effects. However, there is definitely a need to add a dependency to the tablgen generated header files. Sorry for missing this as part of https://reviews.llvm.org/D124851. I would suggest to take a look how this is done for MLIR dialects, where explicits deps to the generated targets are passed to add_mlir_dialect_library like in https://github.com/llvm/llvm-project/blob/04b419048955fc33718ba97e79f3558b6a27830e/mlir/lib/Dialect/EmitC/IR/CMakeLists.txt#L7-L9. It might not be necessary to touch AddLLVM and I assume the deps should rather be on the *IncGen target,

[nikic]

In D125010#3494007, @marbre wrote:

I am currently out-of-office and don‘t have any access to my build machines. Therefore, I cannot test and confirm that the patch doesn‘t have unintended side effects. However, there is definitely a need to add a dependency to the tablgen generated header files. Sorry for missing this as part of https://reviews.llvm.org/D124851. I would suggest to take a look how this is done for MLIR dialects, where explicits deps to the generated targets are passed to add_mlir_dialect_library like in https://github.com/llvm/llvm-project/blob/04b419048955fc33718ba97e79f3558b6a27830e/mlir/lib/Dialect/EmitC/IR/CMakeLists.txt#L7-L9. It might not be necessary to touch AddLLVM and I assume the deps should rather be on the *IncGen target,

I did try just adding MLIRBuiltinAttributeInterfacesIncGen under DEPENDS first, but this ultimately doesn't do anything useful without the change to AddLLVM, because the the intermediate objlib target (for non-ninja generators) essentially means that mlir-pdll depends on both mlir-pdll.cpp.o and MLIRBuiltinAttributeInterfacesIncGen and these are not sequenced (what we need is for mlir-pdll.cpp.o to depend on MLIRBuiltinAttributeInterfacesIncGen).

As to depending directly on *IncGen targets, I can change it in that direction -- my thought here was that the exact dependencies of the headers are something of an implementation detail. But if other areas already hardcode these, then also doing that here makes sense to me as well.

[rriddle]

This looks reasonable to me.

[rriddle]

In D125010#3494133, @nikic wrote:

In D125010#3494007, @marbre wrote:

I am currently out-of-office and don‘t have any access to my build machines. Therefore, I cannot test and confirm that the patch doesn‘t have unintended side effects. However, there is definitely a need to add a dependency to the tablgen generated header files. Sorry for missing this as part of https://reviews.llvm.org/D124851. I would suggest to take a look how this is done for MLIR dialects, where explicits deps to the generated targets are passed to add_mlir_dialect_library like in https://github.com/llvm/llvm-project/blob/04b419048955fc33718ba97e79f3558b6a27830e/mlir/lib/Dialect/EmitC/IR/CMakeLists.txt#L7-L9. It might not be necessary to touch AddLLVM and I assume the deps should rather be on the *IncGen target,

I did try just adding MLIRBuiltinAttributeInterfacesIncGen under DEPENDS first, but this ultimately doesn't do anything useful without the change to AddLLVM, because the the intermediate objlib target (for non-ninja generators) essentially means that mlir-pdll depends on both mlir-pdll.cpp.o and MLIRBuiltinAttributeInterfacesIncGen and these are not sequenced (what we need is for mlir-pdll.cpp.o to depend on MLIRBuiltinAttributeInterfacesIncGen).

As to depending directly on *IncGen targets, I can change it in that direction -- my thought here was that the exact dependencies of the headers are something of an implementation detail. But if other areas already hardcode these, then also doing that here makes sense to me as well.

Your thought is right here, the IncGen should already be provided via MLIRIR. Most IncGen shouldn't be depended on directly.

llvm/cmake/modules/AddLLVM.cmake

macro(add_llvm_executable name)
   if(LLVM_ENABLE_OBJLIB)
     # Generate an obj library for both targets.
     set(obj_name "obj.${name}")
     add_library(${obj_name} OBJECT EXCLUDE_FROM_ALL
       ${ALL_FILES}
       )
     llvm_update_compile_flags(${obj_name})
     set(ALL_FILES "$<TARGET_OBJECTS:${obj_name}>")
+    if(ARG_DEPENDS)
+      add_dependencies(${obj_name} ${ARG_DEPENDS})
+    endif()
 
     set_target_properties(${obj_name} PROPERTIES FOLDER "Object Libraries")
   endif()
 
   add_windows_version_resource_file(ALL_FILES ${ALL_FILES})
 
   if(XCODE)
     # Note: the dummy.cpp source file provides no definitions. However,

mlir/tools/mlir-pdll/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Demangle
   Support
   TableGen
 )
 
-add_tablegen(mlir-pdll MLIR_PDLL
-  mlir-pdll.cpp
-  )
-
-set_target_properties(mlir-pdll PROPERTIES FOLDER "Tablegenning")
-target_link_libraries(mlir-pdll
-  PRIVATE
-    MLIRIR
-    MLIRPDLLAST
-    MLIRPDLLCodeGen
-    MLIRPDLLODS
-    MLIRPDLLParser
+set(LIBS
   )
 
+add_tablegen(mlir-pdll MLIR_PDLL
+  mlir-pdll.cpp
+
+  DEPENDS
+  ${LIBS}
+  )
+
+set_target_properties(mlir-pdll PROPERTIES FOLDER "Tablegenning")
+target_link_libraries(mlir-pdll PRIVATE ${LIBS})
+
 mlir_check_all_link_libraries(mlir-pdll)