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

Various improvements to the compilation process #621

Merged
merged 2 commits into from
Aug 26, 2020

Conversation

mantognini
Copy link
Contributor

I've noticed some issues with the build process. Namely, the available options were not the same for a static build than for a shared build. For example, the --MultiVersionUBOFunctions option of clspv-opt wasn't available in a static build. I had exactly the same issue for the pass I want to introduce for #613 so here is a general fix.

I also had to fix compilation for shared build. I tested this using cmake --target check-spirv with the four {Debug,Release} x {shared,static} configurations on Linux.

Below are the commit messages for the two commits in this PR.


  1. Improve support for shared build

Fix compilation for clspv-reflection when doing a shared-library build.

  • Add clspv_passes to the public list of dependencies of clspv_core so
    that clspv-opt, clspv-reflection and any other target depending on
    clspv_core don't have to link against the dependencies of
    clspv_passes.
  • Remove unnecessary dependencies of clspv-opt now that they are
    transitively added thanks to the previous point.
  • Move clangCodeGen dependency to clspv_core because it isn't used by
    clsvp_passes.
  • Because SPIRV-Tools-shared has hidden symbol visibility, some symbols
    are undefined when linking against SPIRV-Tools (which is an alias for
    SPIRV-Tools-shared or SPIRV-Tools-static based on build options).
    This is worked around by always linking against SPIRV-Tools-static,
    regardless of the build options.
  • Don't explicitly list LLVM components as dependencies as this is
    resolved automatically by CMake.
  1. Improve support for static build

Use OBJECT library for clspv_passes to ensure all the symbols for global
static variables are present in clspv-opt, ensuring passes using a
static RegisterPass<> object are registered. This is effectively
equivalent to using the linker option --whole-archive for static build,
but in a more portable way as not all toolchains support this option.

Previously, the output of clspv-opt --help-hidden and clspv --help-hidden
would differ from a shared library build to a static
library build. There are still some differences but they seem to be
unrelated to clspv passes but rather related to LLVM passes.

The MultiVersionUBOFunctionsPass pass is using such a global static
object. Update existing test to cover this pass.

Bump required CMake version. Chose 3.13.4 as it is required by the
latest LLVM and it supports linking against OBJECT libraries.

Add a missing dependency of clspv_passes that was identified by using
OBJECT library when using a shared build.

Fix compilation for clspv-reflection when doing a shared-library build.
 - Add clspv_passes to the public list of dependencies of clspv_core so
   that clspv-opt, clspv-reflection and any other target depending on
   clspv_core don't have to link against the dependencies of
   clspv_passes.
 - Remove unnecessary dependencies of clspv-opt now that they are
   transitively added thanks to the previous point.
 - Move clangCodeGen dependency to clspv_core because it isn't used by
   clsvp_passes.
 - Because SPIRV-Tools-shared has hidden symbol visibility, some symbols
   are undefined when linking against SPIRV-Tools (which is an alias for
   SPIRV-Tools-shared or SPIRV-Tools-static based on build options).
   This is worked around by always linking against SPIRV-Tools-static,
   regardless of the build options.
 - Don't explicitly list LLVM components as dependencies as this is
   resolved automatically by CMake.

Signed-off-by: Marco Antognini <marco.antognini@arm.com>
Use OBJECT library for clspv_passes to ensure all the symbols for global
static variables are present in clspv-opt, ensuring passes using a
static RegisterPass<> object are registered. This is effectively
equivalent to using the linker option --whole-archive for static build,
but in a more portable way as not all toolchains support this option.

Previously, the output of `clspv-opt --help-hidden` and `clspv
--help-hidden` would differ from a shared library build to a static
library build. There are still some differences but they seem to be
unrelated to clspv passes but rather related to LLVM passes.

The MultiVersionUBOFunctionsPass pass is using such a global static
object. Update existing test to cover this pass.

Bump required CMake version. Chose 3.13.4 as it is required by the
latest LLVM and it supports linking against OBJECT libraries.

Add a missing dependency of clspv_passes that was identified by using
OBJECT library when using a shared build.

Signed-off-by: Marco Antognini <marco.antognini@arm.com>
@dneto0
Copy link
Collaborator

dneto0 commented Aug 25, 2020

Because SPIRV-Tools-shared has hidden symbol visibility, some symbols
are undefined when linking against SPIRV-Tools (which is an alias for
SPIRV-Tools-shared or SPIRV-Tools-static based on build options).

Should this be considered a bug in the list of visible items in the SPIRV-Tools-shared library?
What symbols are missing?

@mantognini
Copy link
Contributor Author

I see these undefined references:

tools/reflection/CMakeFiles/clspv-reflection.dir/main.cpp.o: In function `main':
PATH/clspv/tools/reflection/main.cpp:114: undefined reference to `spvtools::SpirvTools::SpirvTools(spv_target_env)'
PATH/clspv/tools/reflection/main.cpp:115: undefined reference to `spvtools::SpirvTools::Validate(std::vector<unsigned int, std::allocator<unsigned int> > const&) const'
PATH/clspv/tools/reflection/main.cpp:117: undefined reference to `spvtools::SpirvTools::~SpirvTools()'
tools/reflection/CMakeFiles/clspv-reflection.dir/ReflectionParser.cpp.o: In function `clspv::ParseReflection(std::vector<unsigned int, std::allocator<unsigned int> > const&, spv_target_env, std::ostream*)':
PATH/clspv/tools/reflection/ReflectionParser.cpp:337: undefined reference to `spvtools::Context::Context(spv_target_env)'
PATH/clspv/tools/reflection/ReflectionParser.cpp:338: undefined reference to `spvtools::Context::SetMessageConsumer(std::function<void (spv_message_level_t, char const*, spv_position_t const&, char const*)>)'
PATH/clspv/tools/reflection/ReflectionParser.cpp:341: undefined reference to `spvtools::Context::CContext()'
PATH/clspv/tools/reflection/ReflectionParser.cpp:337: undefined reference to `spvtools::Context::~Context()'

These symbols are present in both libSPIRV-Tools.a and libSPIRV-Tools-shared.so but they are defined as local in the latter.

Yet, these are declared in include/spirv-tools/libspirv.hpp so I would assume they should be exported. Would you expect the same? If so, I can create a bug report against the SPIR-V Tools.

@alan-baker alan-baker merged commit 858ff31 into google:master Aug 26, 2020
@mantognini mantognini deleted the cmake branch August 27, 2020 08:31
@mantognini
Copy link
Contributor Author

Thanks for merging.

Re. visibility of symbols: after digging into SPIR-V Tools issues, I've found this comment which seems relevant (i.e. confirming the symbols are expected to be visible):

SPIRV-Tools can be properly generated and consumed as a shared lib (with C and C++ API) if SPIRV_TOOLS_EXPORT is added in front of its C++ API (in include/spirv-tools/libspirv.hpp)

@dneto0
Copy link
Collaborator

dneto0 commented Sep 2, 2020

Yet, these are declared in include/spirv-tools/libspirv.hpp so I would assume they should be exported. Would you expect the same? If so, I can create a bug report against the SPIR-V Tools.

Yes, they should be exported. Thanks for finding this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants