This is an archive of the discontinued LLVM Phabricator instance.

Move the IR and Interfaces library to a new mlir/core subdirectory (NFC)
AcceptedPublic

Authored by mehdi_amini on Mar 13 2022, 5:57 PM.

Diff Detail

Event Timeline

mehdi_amini created this revision.Mar 13 2022, 5:57 PM
mehdi_amini requested review of this revision.Mar 13 2022, 5:57 PM

Can you refresh on what all is intended to go into this core/ directory?

The original proposal had:

        Parser/
        Pass/
        PDL/
        Rewrite/
        Support/
        TableGen/
``

But I haven't checked if there has been new directories recently, I was planning to send reviews one directory at a time

We need some coordination on the CMake include changes. Left some comments but also open to patching what you have and poking at it.

mlir/CMakeLists.txt
144

Why wouldn't it be practical? I think if you add the generated binary include dir at the library level similar to how adding the source include, it should work (or it will once the AddLLVM.cmake changes in https://reviews.llvm.org/D120788 go in).

mlir/core/IR/CMakeLists.txt
60

This isn't quite sufficient.

We will also at least need the changes to llvm_add_library from https://reviews.llvm.org/D120788 (sorry meant to triage the build issues and land this ahead of you).

If we're going this way for the whole tree, then I think I should back out the $<BUILD_INTERFACE:${MLIR_SOURCE_DIR}/include> from the above patch (i.e. only auto-add the the generated files include dir in add_mlir_library). Thoughts?

61

This should be:

$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../include>
mlir/core/Interfaces/CMakeLists.txt
16

I think you need to set target_include_directories here and below as well.

mlir/core/include/CMakeLists.txt
1

What is your expectation that these include_directories are doing?

mlir/CMakeLists.txt
144

I think it is actually pretty important that we fix this before submission so that these include_directories are not required for the new core directories. If they are excluded, then the in-tree includes will be a good mirror/test as for out of tree usage. As written, every out of tree may need to specify these manually in some way if something is gotten wrong.

If it fails in-tree with these removed, that is an indication that we just broke everyone else :)

mehdi_amini added inline comments.Mar 13 2022, 6:40 PM
mlir/CMakeLists.txt
144

The problem I see is every single call to mlir_tablegen need to express a dependency on some target in core to get the include path somehow to resolve things like mlir/IR/OpBase.td.

mlir/core/IR/CMakeLists.txt
60

I think both target_include_directories path in your patch becomes obsolete then, the generated binary files are also in a different paths?

I'm don't really understand the CMake model of how this all interacts with out-of-tree users unfortunately.

61

Oh right!

mlir/core/Interfaces/CMakeLists.txt
16

Right.
It works now because there is a dependency here on MLIRIR which already provides the include path.

mlir/core/include/CMakeLists.txt
1

Oh this is some leftover test I was doing, this is not doing anything useful.

Could the RFC be updated with the final state planned here, intermediate steps that will be followed etc? Currently it is unclear from the RFC what state is (e.g., RFC seems unfinished/conversation just ended 27 days ago seemingly open ended), the end state unclear (as seen from review comments) and steps to get there unknown (e.g., is this just breaking changes in a row?). Judging each individually is convenient thanks. Test file locations is still a bit unclear too: seems like this is changing dir hierarchy but not affecting tests which is curious as I thought a goal was to have the association be easier, and for out of tree to be able to follow the structure closer.

mlir/core/include/mlir/Interfaces/DataLayoutInterfaces.h
15

Shouldn't header guards be updated?

Address Stella's comments

mehdi_amini marked 5 inline comments as done.Mar 13 2022, 6:58 PM
mehdi_amini added inline comments.
mlir/CMakeLists.txt
144

I "fixed" it by removing it from here and moving it in mlir_tablegen : see in mlir/cmake/modules/AddMLIR.cmake.

mlir/cmake/modules/AddMLIR.cmake
7

@stellaraccident : I'm not sure this has a downside?

8

This is unrelated to the current patch, I'll try to land this ahead.

mlir/core/include/mlir/Interfaces/DataLayoutInterfaces.h
15

From the install path point of view, nothing changed?

mlir/CMakeLists.txt
144

Ugh, the tablegen macros are... esoteric and I recall them being weird.

So here is the relevant bit from TableGen.cmake:

get_directory_property(tblgen_includes INCLUDE_DIRECTORIES)
list(TRANSFORM tblgen_includes PREPEND -I)

My personal opinion: we should extend the tablegen function to allow its include directories to be set in another way and sever this dependency on directory-level include_directories. Options here include:

  • extending the current state a bit more and consulting any TABLEGEN_INCLUDES variable in scope (in addition to the directory property)
  • using source file properties in some way (not recommended)
  • teaching tablegen() how to parse arguments like everything else does and have an INCLUDES section on it.

So we also have this lovely thing going on:

function(mlir_tablegen ofn)
  tablegen(MLIR ${ARGV})
  set(TABLEGEN_OUTPUT ${TABLEGEN_OUTPUT} ${CMAKE_CURRENT_BINARY_DIR}/${ofn}
      PARENT_SCOPE)
  include_directories(${CMAKE_CURRENT_BINARY_DIR})
endfunction()

That last include_directories gives me the creeps and we should figure out how to excise it. I think that is explaining why I sometimes see redundant include directories on the tablegen invocations (since multiple calls to tablegen in the same directory will just blindly append the current binary directory INCLUDE_DIRECTORIES directory property multiple times).

I do think that we need to fix this before landing this change: This is already really fragile for out of tree users and making it worse means that everyone adds yet more opaque top level includes that don't make a lot of sense to their projects.

mlir/core/IR/CMakeLists.txt
60

I think both target_include_directories path in your patch becomes obsolete then, the generated binary files are also in a different paths?

I'm don't really understand the CMake model of how this all interacts with out-of-tree users unfortunately.

Yes, was just following up with Marius on that patch and think it should not be needed if being explicit like this. I'll abandon that patch and apply the fixes to llvm/ libraries separately.

The general idea is that you need to make sure that all of the paths needed to include headers make it into INTERFACE_INCLUDE_DIRECTORIES on each library target. The usual way to do that is to call target_include_directories(tgt PUBLIC ...). This appends to both the INTERFACE_INCLUDE_DIRECTORIES and INCLUDE_DIRECTORIES properties on the target. The first is used by libraries that depend on this one. The second is used to build the libraries. Just calling include_directories(...) at the directory level means that every target created in that directory or below will magically have those directories added to its INCLUDE_DIRECTORIES property. This is sufficient to build but offers nothing to people depending/using it.

So you really want to be in a world where you are only using target_include_directories(tgt PUBLIC ...) with no usage of directory level include_directories(...) as that treats all users the same (in tree or out of tree). There may also be occasional need to use target_include_directories(tgt PRIVATE ...) for truly implementation-only include directories, but I don't think we have any of those in practice for this project.

There is another wrinkle: out of tree *installed* users will also get their include directories from INTERFACE_INCLUDE_DIRECTORIES. So typically, we would also want to have something like $<INSTALL_INTERFACE:include>. We may just want to add that automatically in add_mlir_library with something like:

target_include_directories(${name} INTERFACE "$<INSTALL_INTERFACE:include>")

I think we can get away with just doing it the once in the global macro because we squash all include trees down to the one for installation. I didn't recommend doing this in this patch because it isn't regressing the current state to exclude it (there is already some global CMake-ism that slides it in there in that case).

mlir/CMakeLists.txt
144

Ok, that is better (it at least won't apply to the whole tree, which will hide bugs).

mlir/cmake/modules/AddMLIR.cmake
7

Yeah, see my comment that we just mid-air collided on. These are not just setting variables in scope. They are appending to the current directory INCLUDE_DIRECTORIES property, which is global. So every time we call mlir_tablegen in a directory, it will get two more. If this is just to get us out of a weird state, we can probably live with it, but better to create a way to directly instruct the tablegen() function to do exactly what we want without it consulting global properties.

mehdi_amini marked 3 inline comments as done.Mar 13 2022, 9:34 PM
mehdi_amini added inline comments.
mlir/core/IR/CMakeLists.txt
60

The general idea is that you need to make sure that all of the paths needed to include headers make it into INTERFACE_INCLUDE_DIRECTORIES on each library target.

Right. Now how do you translate this to TableGen though?

The thing is that we don't even declare the DEPENDS right now since we rely on the global include flag set in the top level CMakeLists as far as I can tell. If this was a C++ library, we'd have the tablegen invocation that has an #include "mlir/IR/OpBase.td" also has a DEPENDS on a target that provide the include path.

We may have to start by revamping the whole TableGen cmake machinery to work closer to regular libraries? (that is to begin with: setting up and reading the "properties" set on dependencies)

mstorsjo removed a subscriber: mstorsjo.Mar 13 2022, 9:56 PM
mehdi_amini marked an inline comment as done.
mehdi_amini added inline comments.
mlir/cmake/modules/AddMLIR.cmake
7

PTAL!

mehdi_amini marked 3 inline comments as done.Mar 13 2022, 10:08 PM
mlir/cmake/modules/AddMLIR.cmake
7

So for the interim, we just hard-code all tablegen directories here? I can live with that until we get the refactor landed and can then turn these into more of first class targets (which is what we do in IREE, and it makes a lot of the kind of weird dependency issues we have upstream impossible).

Does this actually work? (or is this dependent on an update to TableGen.cmake that is separate?)

mlir/core/IR/CMakeLists.txt
60

I'd like to aim for revamping it (these concepts can be translated to tablegen but it was never really integrated right), but would like to unblock this work with something minimal to get started with. The directory structure you have proposed is a lot more amenable to something better and if we can get that minimally working, we can then come back and revamp.

stellaraccident accepted this revision.Mar 13 2022, 10:36 PM

Found the dependent https://reviews.llvm.org/D121568. With that, these CMake changes look good to me (at least to the level of unblocking the refactor -- I think we will want to come back and clean some things up when you are done).

This revision is now accepted and ready to land.Mar 13 2022, 10:36 PM
rriddle requested changes to this revision.Mar 13 2022, 10:45 PM

I'd prefer we resolve Jacques comments, and get a good understanding of what the final plan is before moving forward with anything (adding a block until then).

This revision now requires changes to proceed.Mar 13 2022, 10:45 PM

I'd prefer we resolve Jacques comments, and get a good understanding of what the final plan is before moving forward with anything (adding a block until then).

By "final plan" here, I don't mean the perfect state of the world. I'm more interested in communicating with the community what is happening right now, and the path we will take moving forward (even if it's incremental).

rriddle accepted this revision.Mar 13 2022, 11:12 PM

LGTM on the patch itself, but I think core/ should retain a lib/ directory. I don't see a huge cost in doing so, and it has a big benefit (IMO) of maintaining the separation between public/private API.

Relaying the current plan to the community though is still very important to me here, and I think that should be done before we starting submitting patches.

This revision is now accepted and ready to land.Mar 13 2022, 11:12 PM

(agreed with River: my acceptance is of the cmake changes specifically, which look good and I think are big step forward for the project)

Fix standalone test

Move under core/lib/...