Page MenuHomePhabricator

[mlir] fix `add_tablegen()` macro to allow installing mlir-pdll
ClosedPublic

Authored by ashay-github on Fri, Aug 5, 12:24 PM.

Details

Summary

Prior to this patch, the add_tablegen() macro in
llvm/cmake/modules/TableGen.cmake added the install rule only if
project matched LLVM or MLIR. This patch adds an optional
DESTINATION argument, which, if non-empty, decides whether (and where)
to install the tablegen tool, thus eliminating the need for
project-specific overrides. This patch also updates all other
invocations of the add_tablegen() macro.

Diff Detail

Event Timeline

ashay-github created this revision.Fri, Aug 5, 12:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Aug 5, 12:24 PM
ashay-github requested review of this revision.Fri, Aug 5, 12:24 PM
nikic added a comment.Fri, Aug 5, 12:43 PM

Funny that. I have a patch in testing that changes the same line to add an OR ${project} STREQUAL CLANG check, because clang-tblgen also needs to be exported. I think this indicates that this hardcoded list doesn't really make sense anymore, and we should instead allow the add_tablegen() call to specify whether this tablegen target should be exported or not. Something along the lines of add_tablegen(mlir-pdll MLIR_PDLL EXPORT) or add_tablegen(mlir-pdll MLIR_PDLL INSTALL).

Or, now that I look through the add_tablegen() calls we have, maybe we should just drop the project check entirely? This is still guarded by LLVM_BUILD_UTILS, so in any case people need to go out of their way to actually install tablegen binaries. (LLVM_BUILD_UTILS defaults to ON for the LLVM project, but will not get set when using a different build system entry point, e.g. a runtimes build.)

ashay-github added a comment.EditedFri, Aug 5, 1:00 PM

Thanks, that makes sense. How about something like add_tablegen(mlir-pdll INSTALL) (i.e. drop the project but add a boolean argument), so that we can still control the installation but not worry about the project name?

Removed the project check.

I'm not sure if the test failures (timeouts) are related to the changes in this patch.

Timed Out Tests (2):
  AddressSanitizer-x86_64-linux :: TestCases/scariness_score_test.cpp
  AddressSanitizer-x86_64-linux-dynamic :: TestCases/scariness_score_test.cpp

Testing Time: 640.70s
  Skipped          :    61
  Unsupported      :  2188
  Passed           : 93982
  Expectedly Failed:   281
  Timed Out        :     2
nikic added inline comments.Sat, Aug 6, 1:29 PM
llvm/cmake/modules/TableGen.cmake
202

Hm, a problem is that this line is going to look for ${MLIR_PDLL_TOOLS_INSTALL_DIR}, so that variable would have to be set (presumably to ${MLIR_TOOLS_INSTALL_DIR}), otherwise this will end up in some kind of default location. The same problem would exist for (LLDB|LIBC|LLVM_LIBC)_TOOLS_INSTALL_DIR, all the other ones already define the variable.

Defining MLIR_PDLL_TOOLS_INSTALL_DIR is possible, but does seem somewhat ugly. A possibility would be to accept the install path as an argument (and skip installing if not provided):

add_tablegen(mlir-pdll MLIR_PDLL ... DESTINATION ${MLIR_TOOLS_INSTALL_DIR})

Added optional DESTINATION argument to add_tablegen(). Also updated all
invocations of the macro.

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSun, Aug 7, 3:48 AM
ashay-github marked an inline comment as done.Sun, Aug 7, 3:49 AM
ashay-github added inline comments.
llvm/cmake/modules/TableGen.cmake
202

Thanks, that seems a lot more elegant than what I had before! Updated in the most recent version.

nikic accepted this revision.Sun, Aug 7, 4:01 AM

LGTM

llvm/cmake/modules/TableGen.cmake
147

I think it would be fine to write this as just cmake_parse_arguments(ADD_TABLEGEN "" "OPTIONS" "" ${ARGN}), but don't feel strongly about that.

This revision is now accepted and ready to land.Sun, Aug 7, 4:01 AM
ashay-github marked an inline comment as done.

Simply invocation of cmake_parse_arguments().

llvm/cmake/modules/TableGen.cmake
147

Thanks! Rewrote it as cmake_parse_arguments(ADD_TABLEGEN "" "DESTINATION" "" ${ARGN})

ashay-github edited the summary of this revision. (Show Details)Sun, Aug 7, 12:23 PM
This revision was landed with ongoing or failed builds.Sun, Aug 7, 3:49 PM
This revision was automatically updated to reflect the committed changes.