Page MenuHomePhabricator

[MLIR] introduce and use add_mlir_{tool,executable} variants
AcceptedPublic

Authored by dtzWill on Apr 21 2022, 3:31 PM.

Details

Summary
  • Identify MLIR targets
  • Optionally don't build tools
  • per-project installation directory (useful for standalone)

Diff Detail

Event Timeline

dtzWill created this revision.Apr 21 2022, 3:31 PM
Herald added a project: Restricted Project. · View Herald Transcript
dtzWill requested review of this revision.Apr 21 2022, 3:31 PM
dtzWill added a comment.EditedApr 21 2022, 3:36 PM

FWIW this isn't quite enough for MLIR standalone but helps get things there. One issue is that the use of LLVM's TableGen.cmake instead of our own means that mlir-tblgen wants to be installed to LLVM_TOOLS_INSTALL_DIR which isn't great when building MLIR separately from LLVM (and keeping installation paths separate, such as for packaging purposes).

The cmake bits are certainly copies of equivalents in LLVM, which I don't love but seems to be what other projects do as well.

(IIRC, similarly, downstream projects using MLIR that try using a standalone-built MLIR will have trouble finding its mlir-tblgen if not among the LLVM tools? Maybe that's just the way I've temporarily resolved the above issue)

I am not really a competent CMake reviewer (just know enough to be dangerous), @mehdi_amini do you have an idea of someone who could competently review this?

Thanks!

I wasn't sure who to ask either.

FWIW https://reviews.llvm.org/D124208 is also in need of a reviewer and is of a similar nature (CMake-only changes, helps build behavior building standalone).

stellaraccident accepted this revision.May 3 2022, 8:01 PM

I am not really a competent CMake reviewer (just know enough to be dangerous), @mehdi_amini do you have an idea of someone who could competently review this?

Cmake is dangerous. Knowing more or less makes no difference :)

This looks good to me and has needed to be done for a while. We may want to tweak the macro more but having it is step one. Thank you.

Are you able to land or do you need assistance with that?

This revision is now accepted and ready to land.May 3 2022, 8:01 PM

FWIW this isn't quite enough for MLIR standalone but helps get things there. One issue is that the use of LLVM's TableGen.cmake instead of our own means that mlir-tblgen wants to be installed to LLVM_TOOLS_INSTALL_DIR which isn't great when building MLIR separately from LLVM (and keeping installation paths separate, such as for packaging purposes).

The cmake bits are certainly copies of equivalents in LLVM, which I don't love but seems to be what other projects do as well.

(IIRC, similarly, downstream projects using MLIR that try using a standalone-built MLIR will have trouble finding its mlir-tblgen if not among the LLVM tools? Maybe that's just the way I've temporarily resolved the above issue)

TableGen is special ina few ways that I would need to take a closer look at to identify issues. Happy to do so, but can we summarize problems on the discussion to look more at of needed?

per-project installation directory (useful for standalone)

Can you help me figuring out what this means? (which part of the code change does something about this here?)

per-project installation directory (useful for standalone)

Can you help me figuring out what this means? (which part of the code change does something about this here?)

Doesn't look like it applies to this patch. Maybe leaking from a related patch.

per-project installation directory (useful for standalone)

Can you help me figuring out what this means? (which part of the code change does something about this here?)

Doesn't look like it applies to this patch. Maybe leaking from a related patch.

Thanks for the review!

Honestly, I thought you might be right :)...

However I believe that this does apply here: using add_llvm_tool/add_llvm_executable from an already installed LLVM causes those targets to try to be installed to LLVM's tool bin directory, which when packaging isn't always appropriate (or writable).

See: https://github.com/llvm/llvm-project/blob/cbfa85734632ad7f961b18539464e7e870f80fd6/llvm/cmake/modules/AddLLVM.cmake#L1255
and
LLVM_TOOLS_INSTALL_DIR is set in the generated LLVMConfig.cmake using this template:
https://github.com/llvm/llvm-project/blob/cbfa85734632ad7f961b18539464e7e870f80fd6/llvm/cmake/modules/LLVMConfig.cmake.in#L124 .
The code added in this change uses CMAKE_INSTALL_BINDIR which is what that variable defaults to in LLVM's build: https://github.com/llvm/llvm-project/blob/cbfa85734632ad7f961b18539464e7e870f80fd6/llvm/CMakeLists.txt#L307 .

I think this also means downstream projects using MLIR's version will work standalone more easily too, but I forget if I tested that.

As for TableGen I'll get back to you. Basically more bits where it expects/searches/generates targets using LLVM paths or so, but I didn't find a good way to fix that served all use cases. For now I think I copy the installed cmake modules, sed-fu them, and point projects at that. I'll try to bring it up for discussion with more details soon!

Thanks. I'm fine landing this as a starting point. Having a project specific macro, even if it doesn't do everything we want yet, is a step forward.