This is an archive of the discontinued LLVM Phabricator instance.

Adjust libMLIR building to more closely follow libClang
ClosedPublic

Authored by vchuravy on Apr 23 2020, 4:41 PM.

Details

Summary
  • Exports MLIR targets to be used out-of-tree.
  • mimicks add_clang_library and add_flang_library.
  • Fixes libMLIR.so

After https://reviews.llvm.org/D77515 libMLIR.so was no longer containing
any object files. We originally had a cludge there that made it work with
the static initalizers and when switchting away from that to the way the
clang shlib does it, I noticed that MLIR doesn't create a obj.{name} target,
and doesn't export it's targets to lib/cmake/mlir.

This is due to MLIR using add_llvm_library under the hood, which adds
the target to llvmexports.

Depends on D78771

Diff Detail

Event Timeline

vchuravy created this revision.Apr 23 2020, 4:41 PM
vchuravy edited projects, added Restricted Project; removed Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2020, 4:42 PM
mlir/cmake/modules/AddMLIR.cmake
87

I think dependency problem is solved if you change INTERFACE to PUBLIC here.

mlir/tools/mlir-shlib/CMakeLists.txt
35

I think MLIR almost universally puts this on the previous line

mlir/cmake/modules/AddMLIR.cmake
87

Actually, nevermind, that's not going to help anything.

mlir/tools/mlir-shlib/CMakeLists.txt
39–46

Insert here:
+ foreach (lib ${mlir_libs})
+ add_dependencies(MLIR ${lib})
+ endforeach ()
seems to solve the dependency problem. (Not fully tested) I suspect the problem is that deep in the bowels of cmake it handles the dependencies different for object libraries. These seem to not quite get the same dependency handling as target_link_libraries.

mlir/tools/mlir-shlib/CMakeLists.txt
39–46

I see however, that this doesn't seem to work with makefiles. (it seems that flang also suffers from this makefile problem)

DavidTruby resigned from this revision.Apr 24 2020, 7:41 AM
vchuravy updated this revision to Diff 259902.Apr 24 2020, 9:07 AM

small fixes and use MLIR_STATIC_LIBS

vchuravy marked 4 inline comments as done.Apr 24 2020, 9:09 AM

With ninja all dependencies edges are fine, but with Makefiles it indeed still complains about missing edges to the generated headers.

Does this get us closer to fixing LLVM_LINK_LLVM_DYLIB?

mlir/cmake/modules/MLIRConfig.cmake.in
22

This was solving a specific problem. Are you removing it because you believe that the problem is now solved a different way?

vchuravy marked an inline comment as done.Apr 24 2020, 10:49 AM

Does this get us closer to fixing LLVM_LINK_LLVM_DYLIB?

Haven't tried yet, but I think it is a step in the right direction. I think

target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})

is necessary for that

mlir/cmake/modules/MLIRConfig.cmake.in
22

Yes we were never creating a MLIR_CONFIG_EXPORTS_FILE. With the changes to add_mlir_library we are now actually creating the file and are no longer polluting the LLVM_CONFIG_EXPORTS_FILE

I think this is at least a step in the right direction.

It looks like the make problems may be more fundamental. CMake generates recursive makefiles, but these somewhat implicitly assume that hierarchical directories are processed in order (e.g. mlir/include before mlir/lib) It seems that there is a somewhat fundamental problem with the current organization because mlir-linalg-ods-gen depends on MLIRAnalysis and MLIRParser, but these also depend on the dialects., but the linalg dialect depends on MLIRAnalysis to get the verifier. There currently a 'hack' for mlir-tblgen which builds it independently of the other tools, but it's relatively dependence-free so that works easily. To get mlir-linalg-ods-gen to work we'd need to do something similar for:

lib/Analysis/Verifier.cpp
lib/IR
lib/Parser

Even fixing mlir-headers so that all tablegen runs happen first doesn't work with makefiles because of the above subdirectory ordering problem .

This revision is now accepted and ready to land.Apr 24 2020, 12:21 PM

@vchuravy, What's your plan with this patch? I think this should land and I've started building on top of it.

@vchuravy, What's your plan with this patch? I think this should land and I've started building on top of it.

I don't think this can land until we resolve one way or the other https://reviews.llvm.org/D78771?

My understanding is that D78771 is trying to solve a dependency problem for header files. Is that situation made worse by 78773?
Maybe you can also take a look at https://reviews.llvm.org/D79067?

I looked into the dependency problem. There is a significant change here because this moves to generating object libraries in most cases. This changes the way the dependencies are handled. I'm working on a fix.

stephenneuendorffer edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.