[mlir] Fix calling the native mlir-tblgen tool when cross compiling flang

[mstorsjo]

Summary

When the mlir-tblgen tool is set up, the MLIR_TABLEGEN_EXE variable is set,
which either points to the mlir-tblgen tool built in the current cmake build,
or points to one built in a nested cmake build (if cross conpiling, or if building
with e.g. LLVM_OPTIMIZED_TABLEGEN.

The MLIR_TABLEGEN_EXE variable is only set within the scope of the mlir/CMakeLists.txt
file, so it's unavailable in sibling level projects such as flang.

Set the MLIR_TABLEGEN_EXE and the MLIR_TABLEGEN_TARGET variables as global,
so that flang can use them properly without guessing.

My cmake-fu is pretty weak, so I'm open to suggestions on how to do this better.

[jpienaar]

Funnily we were just looking at similar case. I'll leave actual review to folks who know Cmake more.

[powderluv]

mlir/CMakeLists.txt

159

Shouldn't the issue be fixed in add_tablegen() as it says in the comment above so we are not working around that further by re-setting into the cache here.

[powderluv]

I tried the patch locally to build torch-mlir https://github.com/llvm/torch-mlir/issues/1094 and it still fails cross-compile.

[mstorsjo]

In D130350#3684169, @powderluv wrote:

I tried the patch locally to build torch-mlir https://github.com/llvm/torch-mlir/issues/1094 and it still fails cross-compile.

If you apply the to torch-mlir, you also need a corresponding change to the one in flang/CMakeLists.txt in this patch, where set(MLIR_TABLEGEN_EXE $<TARGET_FILE:mlir-tblgen>) is removed - I presume that torch-mlir has something similar.

mlir/CMakeLists.txt

159

Yes, maybe… Apparently, add_tablegen adding the variable to the parent scope has been enough for all other projects so far, with their structure - e.g. where clang-tools-extra is set up as a sub-scope under clang/tools, instead of as a sibling level scope. If we’d make add_tablegen always set it as a global cache variable, it would just work for this kind of hierarchy too.

I just tried to do this as a less intrusive change initially, as mlir seems to be the only one affected so far.

[marbre]

In D130350#3684336, @mstorsjo wrote:

In D130350#3684169, @powderluv wrote:

I tried the patch locally to build torch-mlir https://github.com/llvm/torch-mlir/issues/1094 and it still fails cross-compile.

If you apply the to torch-mlir, you also need a corresponding change to the one in flang/CMakeLists.txt in this patch, where set(MLIR_TABLEGEN_EXE $<TARGET_FILE:mlir-tblgen>) is removed - I presume that torch-mlir has something similar.

Yep, see https://github.com/llvm/torch-mlir/blob/3c9addf19cd5eae62562600294150534dc353eae/CMakeLists.txt#L103.

mlir/CMakeLists.txt

159

I agree that this has to be fixed within add_tablegen() somehow or rather that we need to use add_tablegen() in the correct way. Dropping set(MLIR_TABLEGEN_EXE $<TARGET_FILE:mlir-tblgen>) like in the suggested patch seems a good start. However, I am not sure (yet) if setting MLIR_TABLEGEN_EXE in mlir/CMakeLists.txt is the way this should be fixed (I also haven't had the time yet to test if it).
Looking into add_tablegen() it already does something similar, see https://github.com/llvm/llvm-project/blob/69d5a038b90d3616a5c22d4e0e682aad4679758d/llvm/cmake/modules/TableGen.cmake#L155-L171.

If desired I can take over and look into the issue in more detail next week (I am off tomorrow).

[mstorsjo]

mlir/CMakeLists.txt

159

It’s not about mlir using add_tablegen incorrectly, the issue is in how the scopes are nested hierarchically. add_tablegen sets the variable in the parent scope, so it’s usable there. For the normal cases, it looks like this (slightly simplified):

llvm/
  TableGen/
  lib/
    Target/
      …

Here, in TableGen, the *_EXE variables are set in the parent scope, i.e. in the llvm/ scope. Thus, the variables are available to be used in llvm/lib/Target.

Mlir does it in the same way, and MLIR_TABLEGEN_EXE (pointing to either the normal just built one, or a nested cmake native one) is available and works correctly within the mlir tree.

But as MLIR_TABLEGEN_EXE is set in the scope of the mlir directory, and flang and torch-mlir aren’t hooked into the build within the mlir scope but on the toplevel, the variables set in the mlir scope aren’t visible to them.

(Sorry if that already was obvious - just making sure we’re on the same page here.)

So either flang and torch-mlir need to be included as child projects under mlir (kinda like how clang-tools-extra is hooked in under clang, despite being a toplevel project itself), or the *_TABLEGEN_EXE variables need to be made global (which is done by making them cache variables) instead of scoped. Here I do the latter, but manually instead of changing the definition of add_tablegen.

[marbre]

mlir/CMakeLists.txt

159

Thanks for the detailed response! I am generally aware of the nesting problem but needed to take a deeper look into what exactly add_tablegen does.

I think the suggested patch should work out, but I think this depends on the build type. In case a project is compiled as part of LLVM via the LLVM_EXTERNAL_PROJECTS mechanism, it could be sufficient to add it to the parent scope (haven't tested):

set(MLIR_TABLEGEN_EXE ${MLIR_TABLEGEN_EXE} PARENT_SCOPE)
set(MLIR_TABLEGEN_TARGET ${MLIR_TABLEGEN_TARGET} PARENT_SCOPE)

We might also need to take care of mlir-pdll which is build via add_tablegen as well. But I can do this in a follow-up.

TL;DR: Since the patch should improve our current situation, I think this can be merged as is and is something we can iterate on (if needed). Regarding torch-mlir, I can take a look after this one had landed and we can see if something else is needed.

[mstorsjo]

mlir/CMakeLists.txt

159

I did test set(MLIR_TABLEGEN_EXE ${MLIR_TABLEGEN_EXE} PARENT_SCOPE), and if I remember correctly, it's not enough - it ends up setting the variable within the scope of a function in llvm/tools, it would essentially need to be PARENT_SCOPE^2 to reach a scope where it's visible to flang and other external projects.

I'll try to land this and see how it fares.

[powderluv]

mlir/CMakeLists.txt

159

I think you will need to set

set(MLIR_TABLEGEN_EXE ${MLIR_TABLEGEN_EXE} PARENT_SCOPE)

in each of the directories until you reach the top level so you can get it to PARENT_SCOPE^2.

I was able to test that however NATIVE MLIR_TABLEGEN is still not being used when cross-compiling and may need follow of fixes.

[mstorsjo]

mlir/CMakeLists.txt

159

Yes, it would be possible to have a chain of such statements. In practice, it would need to be in add_llvm_subdirectory (which is called from the macro add_llvm_external_project, from llvm/tools/CMakeLists.txt) - but having such a specific variable handling there would feel very much out of place there.

If you're trying to cross compile a project other than flang, it at least needs a change similar to the one in flang/CMakeLists.txt here, which removes an existing set(MLIR_TABLEGEN_EXE $<TARGET_FILE:mlir-tblgen>). I don't know what project you're compiling or how it is set up, so I can't comment on that.

[powderluv]

mlir/CMakeLists.txt

159

We are building torch-mlir and we just removed the

set(MLIR_TABLEGEN_EXE $<TARGET_FILE:mlir-tblgen>)

just like in flang. The issue is tracked here https://github.com/llvm/torch-mlir/issues/1094. Basically on macOS the NATIVE mlir_tblgen is built but because of some override somewhere we are trying to use the target mlir_tblgen.

flang/CMakeLists.txt

else()
   else()
     set(FLANG_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})
   endif()
 
   set(BACKEND_PACKAGE_STRING "${PACKAGE_STRING}")
   set(MLIR_MAIN_SRC_DIR ${LLVM_MAIN_SRC_DIR}/../mlir/include ) # --src-root
   set(MLIR_INCLUDE_DIR ${LLVM_MAIN_SRC_DIR}/../mlir/include ) # --includedir
   set(MLIR_TABLEGEN_OUTPUT_DIR ${CMAKE_BINARY_DIR}/tools/mlir/include)
-  set(MLIR_TABLEGEN_EXE $<TARGET_FILE:mlir-tblgen>)
   include_directories(SYSTEM ${MLIR_INCLUDE_DIR})
   include_directories(SYSTEM ${MLIR_TABLEGEN_OUTPUT_DIR})
 endif()
 
 set(FLANG_TOOLS_INSTALL_DIR "${CMAKE_INSTALL_BINDIR}" CACHE PATH
     "Path for binary subdirectory (defaults to '${CMAKE_INSTALL_BINDIR}')")
 mark_as_advanced(FLANG_TOOLS_INSTALL_DIR)
 

mlir/CMakeLists.txt

 
 # Adding tools/mlir-tblgen here as calling add_tablegen sets some variables like
 # MLIR_TABLEGEN_EXE in PARENT_SCOPE which gets lost if that folder is included
 # from another directory like tools
 add_subdirectory(tools/mlir-tblgen)
 add_subdirectory(tools/mlir-linalg-ods-gen)
 add_subdirectory(tools/mlir-pdll)
 
+set(MLIR_TABLEGEN_EXE "${MLIR_TABLEGEN_EXE}" CACHE INTERNAL "")
+set(MLIR_TABLEGEN_TARGET "${MLIR_TABLEGEN_TARGET}" CACHE INTERNAL "")
+
 add_subdirectory(include/mlir)
 add_subdirectory(lib)
 # C API needs all dialects for registration, but should be built before tests.
 add_subdirectory(lib/CAPI)
 
 if (MLIR_INCLUDE_TESTS)
   add_definitions(-DMLIR_INCLUDE_TESTS)
   add_custom_target(MLIRUnitTests)