Page MenuHomePhabricator

Extends the tblgen macro to allow mlir-tblgen to be installed
ClosedPublic

Authored by schweitz on Nov 4 2019, 1:28 PM.

Details

Summary

The mlir-tblgen tool was not getting installed. This change allows the MLIR project to be installed along with llvm-tblgen.

Diff Detail

Event Timeline

schweitz created this revision.Nov 4 2019, 1:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2019, 1:28 PM
mehdi_amini added inline comments.Nov 5 2019, 12:01 PM
llvm/cmake/modules/TableGen.cmake
163

Is this correct? Usually OR has lower precedence then AND.

Also that seems a bit intrusive to me. What about replacing the existing if (${project} STREQUAL LLVM part with an extra argument to the macro that indicates from the call site to install or not?

schweitz updated this revision to Diff 227959.Nov 5 2019, 1:41 PM

Thanks for catching the brain cramp.

I don't know about altering the macro generally. I agree that having these package names hard coded here could be improved.

Is there any reason not to install *-tblgen programs by default?

e.g. clang-tblgen is also needed for cross-compilation purposes

When you build/install clang for the host system, the clang-tblgen you would get is not necessarily compatible with the clang you intend to cross-compile. It is also a bit strange to me to expect that to build clang for a device you would need a toolchain that supply clang-tblgen.

Instead, I think the build scripts of clang when cross compiling should include the logic to build a clang-tblgen for the host as well and use it during the build process.

Hi Mehdi,

Do you know who the best person to ask about the cmake build stuff would be?

@beanz or @tstellar are likely knowledgeable on this domain

beanz added a comment.Nov 19 2019, 9:21 AM

When cross compiling any LLVM-based project we do not rely on installed copies of table gen, instead we configure and build host tablegen tools. That functionality in the LLVM build system is flexible enough to support any sub-project that provides its own tablegen tool.

In general we don't install tablegen tools because they are tightly tied to the library-based extensibility. For LLVM we do because we support building LLVM targets against installed copies of LLVM. I'm not familiar enough with MLIR, what is the use case for an installed mlir-tblgen tool?

In general we don't install tablegen tools because they are tightly tied to the library-based extensibility. For LLVM we do because we support building LLVM targets against installed copies of LLVM. I'm not familiar enough with MLIR, what is the use case for an installed mlir-tblgen tool?

I think the use case for mlir-tblgen is very similar to the external backend case of LLVM. Someone who would develop an out-of-tree dialect for MLIR and would like to build/link against a prebuilt MLIR would need to use the mlir-tblgen that comes with the installed MLIR (or build it themselves)

beanz accepted this revision.Nov 19 2019, 12:48 PM

Given that, this change LGTM. Using an extra argument would be nice, but this is a macro, not a function, and IIRC CMake argument parsing in macros doesn't really work, so that would be a big change.

This revision is now accepted and ready to land.Nov 19 2019, 12:48 PM

In general we don't install tablegen tools because they are tightly tied to the library-based extensibility. For LLVM we do because we support building LLVM targets against installed copies of LLVM. I'm not familiar enough with MLIR, what is the use case for an installed mlir-tblgen tool?

I think the use case for mlir-tblgen is very similar to the external backend case of LLVM. Someone who would develop an out-of-tree dialect for MLIR and would like to build/link against a prebuilt MLIR would need to use the mlir-tblgen that comes with the installed MLIR (or build it themselves)

+1

Exactly. We're using mlir-tblgen as a tool to extend MLIR with our dialect. This change is to allow out-of-tree builds to use mlir-tblgen to generate all the dialect extensions, etc.

Can someone commit this for me? Thanks!

This revision was automatically updated to reflect the committed changes.