Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake build isn't respecting ALWAYSLINK property, breaking static registration #190

Closed
ScottTodd opened this issue Dec 13, 2019 · 9 comments
Assignees
Labels
infrastructure Relating to build systems, CI, or testing

Comments

@ScottTodd
Copy link
Collaborator

I'm trying to run parts of IREE's compiler rebased off of #188 (eventually trying to refactor some of the CMake pieces into iree_bytecode_module and related helpers). However, iree-translate doesn't seem to have statically registered translations on it.

In particular, building iree_tools_iree_translate and then running iree-translate --help should be showing a list of options like -mlir-to-iree-module, but the linker (at least on my Linux machine when building via Clang 8.0.1) doesn't seem to be including iree::compiler::Translation::Sequencer in the final binary. The Bazel build of the equivalent target works as expected. The dependency is definitely specified correctly, since compile errors in SequencerModuleTranslation.cpp cause building that target to fail.

MLIR has a whole_archive_link function here that seems to do what we might need, but I wasn't able to get that working with iree_compiler_Translation_Sequencer or the other libraries today (may need to adjust paths/names so the -l can find them?). It looks like there may be other ways to handle this with recent CMake versions, but I ran out of time today to research/experiment further.

Also, "${IREE_ROOT_DIR}/third_party/mlir/tools/mlir-translate/mlir-translate.cpp" should be changed to "iree_translate_main.cc", but that doesn't seem to help with the link issue.

@ScottTodd ScottTodd self-assigned this Dec 16, 2019
@ScottTodd
Copy link
Collaborator Author

Seeing the same behavior on Windows with MSVC.

CMake:

C:\dev\iree                                                                                                             
λ build\iree\tools\iree-translate.exe --help                                                                            
OVERVIEW: MLIR translation driver                                                                                       
                                                                                                                        
USAGE: iree-translate.exe [options] <input file>                                                                        
                                                                                                                
General options:                                                                                                        
                                                                                                                        
  --mlir-elide-elementsattrs-if-larger=<uint> - Elide ElementsAttrs with "..." that have more elements than the given up
per limit                                                                                                               

  ... (other options here) ...

Bazel:

C:\dev\iree
λ bazel run iree/tools:iree-translate -- --help

OVERVIEW: IREE translation driver

USAGE: iree-translate.exe [options] <input file>

General options:

  --disable-pass-threading                              - Disable multithreading in the pass manager
  Translation to perform
      --iree-mlir-to-vm-bytecode-module                    - iree-mlir-to-vm-bytecode-module
      --iree-vm-ir-to-bytecode-module                      - iree-vm-ir-to-bytecode-module
      --mlir-to-iree-module                                - mlir-to-iree-module
  --iree-vm-bytecode-module-optimize                    - Optimizes the VM module with CSE/inlining/etc prior to serialization
  --iree-vm-bytecode-module-output-format=<value>       - Output format the bytecode module is written in
    =flatbuffer-binary                                  -   Binary FlatBuffer file
    =flatbuffer-text                                    -   Text FlatBuffer file, debug-only
    =mlir-text                                          -   MLIR module file with annotations
  --iree-vm-bytecode-module-strip-debug-ops             - Strips debug-only ops from the module
  --iree-vm-bytecode-module-strip-source-map            - Strips the source map from the module
  --iree-vm-bytecode-module-strip-symbols               - Strips all internal symbol names from the module
  --mlir-elide-elementsattrs-if-larger=<uint>           - Elide ElementsAttrs with "..." that have more elements than the given upper limit

  ... (other options here) ...

Linking with --whole-archive appears to generally help (it found a few tensorflow/mlir dependencies that aren't included in the CMake build yet), though I still want to figure out how to apply that option selectively to individual libraries / parts of the build and not just to the entire binary wholesale.

@ScottTodd
Copy link
Collaborator Author

After reading up on CMake and C++ linking, I have something approximating a solution for ALWAYSLINK in our CMake configuration that registers IREE's passes in the iree-translate binary. I'll send a pull request once I can clean up my local commits and test on a few different machines.

The general idea is to put a list of CMake targets between "-Wl,--whole-archive" and "-Wl,--no-whole-archive" in the target_link_libraries command within iree_cc_binary. I also tried per-library link options in iree_cc_library but couldn't find a way to get that to work in the same way.

@marbre
Copy link
Contributor

marbre commented Dec 17, 2019

Thanks @ScottTodd for tackling this and apologies that I did not took care within the CMake configuration. To recap and make sure I got it, ALWAYSLINK will only be respected within executables build with iree_cc_binary. Do we still need to fix this somehow for libraries created by iree_cc_library or is your fix sufficient? Greping for alwayslink in the iree subdirectory, it gives me 40 more hits where this is set in the Bazel BUILD configuration.

@ScottTodd
Copy link
Collaborator Author

Libraries using iree_cc_library will not need changes. External libraries that also need to always be linked in may need special treatment, such as here: https://github.com/google/iree/blob/fdac5e4e55a63e7e7d1e2ef257d1f4200104e350/iree/tools/CMakeLists.txt#L17-L35

Projects depending on IREE (such as to build compiler tools on top of IREE with their own passes included as well) will either need to use iree_cc_binary or devise their own solution to always linking.

Here's a good reference I found for the --whole-archive linking technique: https://github.com/PixarAnimationStudios/USD/blob/4b1162957b2ad219e1635c81de8343be490e41fc/cmake/macros/Private.cmake#L809-L870 . That's generally the approach that I'm taking.

The last piece that I haven't figured out yet is transitive dependencies, which that reference has _pxr_transitive_internal_libraries for, and @benvanik had some ideas for in 0407af7 (old branch).

copybara-service bot pushed a commit that referenced this issue Dec 18, 2019
Progress on #190. iree-translate now includes options such as `-mlir-to-iree-module`, which is needed for building the samples with CMake.

Transitive alwayslink dependencies are not supported yet.

Tested on Linux and Windows. I haven't tested gcc yet, which may need a branch, see https://github.com/PixarAnimationStudios/USD/blob/4b1162957b2ad219e1635c81de8343be490e41fc/cmake/macros/Private.cmake#L809-L870

Closes #205

COPYBARA_INTEGRATE_REVIEW=#205 from ScottTodd:cmake-alwayslink c661044
PiperOrigin-RevId: 286088548
@ScottTodd
Copy link
Collaborator Author

Keeping this issue open to track handling transitive dependencies, but this is otherwise fixed.

@marbre
Copy link
Contributor

marbre commented Jan 2, 2020

I still have some trouble with the ALWAYSLINK property. E.g. trying to build iree::tools::iree-dump-module with the configuration

  iree_cc_binary(
    NAME
      iree-dump-module
    SRCS
      "dump_module_main.cc"
    DEPS
      iree::base::file_io
      iree::base::flatbuffer_util
      iree::base::init
      iree::schemas::bytecode_module_def_cc_fbs
      flatbuffers
  )

results in

CMake Error at build_tools/cmake/iree_cc_binary.cmake:106 (get_target_property):
  INTERFACE_LIBRARY targets may only have whitelisted properties.  The
  property "ALWAYSLINK" is not allowed.
Call Stack (most recent call first):
  iree/tools/CMakeLists.txt:62 (iree_cc_binary)

This caused by depending on iree::schemas::bytecode_module_def_cc_fbs. However, the library iree::schemas::bytecode_module_def_cc_fbs itself does not has the ALWAYSLINK property. Only some other libraries depending on iree::schemas::bytecode_module_def_cc_fbs have, such as iree::compiler::Dialect::VM::Target::Bytecode.
Obviously iree::schemas::bytecode_module_def_cc_fbs needs to be always linked if the dependency is pulled in by iree::compiler::Dialect::VM::Target::Bytecode. But is it also necessary for iree-dump-module or do we need to somehow clear the PROPERTY ALWAYSLINK 1 for those deps or something like this?

@ScottTodd
Copy link
Collaborator Author

Hi, I have Internet access again 🎉. I'll have access to a dev machine again on Monday.

For those errors, I'd look at INTERFACE_LIBRARY handling here:
https://github.com/google/iree/blob/4af17b6b9d99b14addde6447ebd86bb407d33e90/build_tools/cmake/iree_cc_library.cmake#L80-L164

And here:
https://github.com/google/iree/blob/4af17b6b9d99b14addde6447ebd86bb407d33e90/build_tools/cmake/flatbuffer_cc_library.cmake#L95-L111

@marbre
Copy link
Contributor

marbre commented Jan 3, 2020

Thanks for pointing me on that specific snippet. Currently working on some other, CMake related issues, but might have the chance to take a closer look.

@ScottTodd
Copy link
Collaborator Author

Oh, that failure was actually simpler and the error message was pretty helpful - get_target_property fails on non-whitelisted properties, so we should first check if each dep is an INTERFACE_LIBRARY before trying to get the ALWAYSLINK property. PR sent with a fix.

copybara-service bot pushed a commit that referenced this issue Jan 14, 2020
Addresses failure noted at #190 (comment)
Adds configuration for `iree-dump-module` for #393

Closes #464

COPYBARA_INTEGRATE_REVIEW=#464 from ScottTodd:cmake-tools 3b6865d
PiperOrigin-RevId: 289541324
stellaraccident pushed a commit that referenced this issue Sep 24, 2023
* iree: bed2763 Clean up code of legacy benchmark suite (#14081) (Wed
Jun 14 00:12:46 2023 -0400)
* xla: c13f357f0 Reverts 34d661c3b792cd9a55a5f6de6d5b58e4d7856c4b (Wed
Jun 14 11:24:40 2023 -0700)
* jax: b42282d30 Ensure effect indices are updated when constvars are
modified. This resolves a bug where conditional Read<N> effect indices N
were sometimes referring to the incorrect invars. (Wed Jun 14 10:09:54
2023 -0700)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Relating to build systems, CI, or testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants