iree-org / iree

A retargetable MLIR-based machine learning compiler and runtime toolkit.
http://iree.dev/
Apache License 2.0
2.85k stars 614 forks source link

Compiler tool dynamic linking may not be setting CMake dependencies completely #11331

Open ScottTodd opened 1 year ago

ScottTodd commented 1 year ago

Likely culprit: https://github.com/iree-org/iree/pull/11285

I'm noticing that when I make changes to compiler code and rebuild iree-test-deps, the .vmfb test files are not regenerated. I think we're missing a dependency link between IREECompiler.dll (.so on Linux) and iree-compile.exe. The iree_check_test targets depend on the compiler tool here: https://github.com/iree-org/iree/blob/45bd5504a791ba112a4eb52cd963d7915d302ac8/build_tools/cmake/iree_bytecode_module.cmake#L123-L140

Can anyone else confirm similar behavior? This might need more investigation.

ScottTodd commented 1 year ago

I think the PRIVATE linkage here is the problem: https://github.com/iree-org/iree/blob/e36ce110017a6a1988f0f67a4b47e5a374e33e86/compiler/src/iree/compiler/API2/CMakeLists.txt#L76-L87

I don't quite follow that comment (and why a "boundary shared library" is useful within the build system). @stellaraccident ?

I can restore the expected behavior (changing the compiler rebuilds iree-test-deps) by either flipping that to PUBLIC or by inserting a dep on iree::compiler::API2::StaticImpl here (though other places may similarly be "broken"): https://github.com/iree-org/iree/blob/e36ce110017a6a1988f0f67a4b47e5a374e33e86/build_tools/cmake/iree_bytecode_module.cmake#L131-L136

benvanik commented 1 year ago

The goal of the library is to have one dynamic library that all consumers dynamically link against. If you add the static library as PUBLIC then all consumers will also statically link against everything. If you look in your build dir you should see either 50KB for iree-compile/iree-opt/etc + the 400MB (debug) compiler binary if dynamically linking or iree-compile/iree-opt/etc all at 400MB if statically linking.

There should be a straight-line dep here: code -> static library -> shared library -> tools -> test artifacts. The thing to investigate is where that breaks down as code changes should propagate all the way through: code changes, static library picks up those code changes, shared library picks up the static library changes, tools pick up the shared library changes, etc.

Maybe there's something special to get a shared library to invalidate dependent targets? Seems like something we could try to reproduce standalone to see if we're holding cmake right.

ScottTodd commented 1 year ago

(By "inserting a dep on iree::compiler::API2::StaticImpl" I mean to DEPENDS in add_custom_command, which is just for computing when to rebuild - not actually adding any new linking)

RE: PUBLIC vs PRIVATE, https://cmake.org/cmake/help/latest/command/target_link_libraries.html:

Libraries and targets following PUBLIC are linked to, and are made part of the link interface. Libraries and targets following PRIVATE are linked to, but are not made part of the link interface

stellaraccident commented 1 year ago

I think the PRIVATE linkage here is the problem:

https://github.com/iree-org/iree/blob/e36ce110017a6a1988f0f67a4b47e5a374e33e86/compiler/src/iree/compiler/API2/CMakeLists.txt#L76-L87

I don't quite follow that comment (and why a "boundary shared library" is useful within the build system). @stellaraccident ?

I can restore the expected behavior (changing the compiler rebuilds iree-test-deps) by either flipping that to PUBLIC or by inserting a dep on iree::compiler::API2::StaticImpl here (though other places may similarly be "broken"):

https://github.com/iree-org/iree/blob/e36ce110017a6a1988f0f67a4b47e5a374e33e86/build_tools/cmake/iree_bytecode_module.cmake#L131-L136

If you link as public, then the compile options get inherited, which is not what you want for such a C API. I discovered it because the flags to disable exceptions were being added to all dependents when linked public.

I didn't notice the vmfb deps not rebuilding (I could have sworn they were for me). Adding it as a depends should be fine.

This was a bit of a head scratcher...

ScottTodd commented 1 year ago

I didn't notice the vmfb deps not rebuilding (I could have sworn they were for me). Adding it as a depends should be fine.

Just tested on Linux, and I do see them rebuilding correctly with no changes. This might be Windows specific...

ScottTodd commented 1 year ago

I'm not on my Windows machine right now to test, but maybe we could flip this PRIVATE to PUBLIC (looking for what's different on Windows vs Linux): https://github.com/iree-org/iree/blob/da5b7f707bd2828969a779c59c6c1224d72dc189/build_tools/cmake/iree_cc_library.cmake#L110-L115