Page MenuHomePhabricator

[mlir] Support cmake 3.15+ in the python mlir configuration
AbandonedPublic

Authored by stella.stamenova on Jul 5 2022, 3:16 PM.

Details

Summary

https://reviews.llvm.org/D128230 updated the mlir python configuration to depend heavily on interface libraries and their properties. Unfortunately, some of these are not available in cmake 3.15 (which is the lowest supported version for mlir). This change modifies the setup of the interface libraries to work in cmake 3.15.

More specifically, the issue is that both INCLUDE_DIRECTORIES and SOURCES are only supported on interface libraries in 3.15 in their "interface" versions, so we cannot set plain INCLUDE_DIRECTORIES and SOURCES. We were setting these to work around the fact that we "build" the mlir sources targets at configuration time (instead of build time) and the generator expressions are not yet evaluated at that point. This change removes these "plain" variants of the properties and instead adds custom evaluation for the generator expressions. This is not really great (neither was the other workaround, really), so we should plan to upgrade to FILE_SETS or similar as soon as possible (which depends on requiring a much newer version of cmake). Other options include always copying the python files instead of creating symbolic links.

This change also fixes a dependency issue between the python files generated through tablegen and the targets that copy the files. The dependency was previously implied via the SOURCES property, however, there really should be an explicit target dependency per the cmake documentation (see the OUTPUT property of https://cmake.org/cmake/help/latest/command/add_custom_command.html).

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 3:16 PM
stella.stamenova requested review of this revision.Jul 5 2022, 3:16 PM
mlir/cmake/modules/AddMLIRPython.cmake
333

I think this function should be removed entirely and instead declare_mlir_dialect_python_bindings should be modified to support the extension naming scheme because they are almost identical, but I didn't want to do it in this change.

Oof... thank you for looking into this, but this is also pretty gross/fragile. Are we *sure* we want to do this? This is the only project I work on that treats CMake as an LTS-like thing, and I would be utterly shocked if this was the only optional feature that found itself needing a newer CMake for basic things. Do you happen to know which CMake version this workaround is not needed for (I knew at one time but have forgotten)?

Oof... thank you for looking into this, but this is also pretty gross/fragile. Are we *sure* we want to do this? This is the only project I work on that treats CMake as an LTS-like thing, and I would be utterly shocked if this was the only optional feature that found itself needing a newer CMake for basic things. Do you happen to know which CMake version this workaround is not needed for (I knew at one time but have forgotten)?

I think it would be much better to upgrade the CMake version than try to support older versions not just for this but in general! I also did want to make sure we could support 3.15 if we wanted. The interface library works great in 3.15 for the code that we compile (that path has no changes to work in 3.15), but the path with the copy/symlinks is worse as you can see.

I did some testing and 3.18.3 does not work with the current setup, but 3.19 does. I'm not sure if there's a version in between that does.

stella.stamenova abandoned this revision.Tue, Aug 16, 4:19 PM

This is obsolete since https://reviews.llvm.org/D130171 was committed