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

llvmPackages_10: rc2 -> rc3 #81819

Merged
merged 1 commit into from Mar 11, 2020
Merged

llvmPackages_10: rc2 -> rc3 #81819

merged 1 commit into from Mar 11, 2020

Conversation

ggreif
Copy link
Contributor

@ggreif ggreif commented Mar 5, 2020

Motivation for this change

http://lists.llvm.org/pipermail/llvm-dev/2020-March/139729.html

Things done

such that clang can automatically pick up the polly plugin from the
llvm-polly build.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ggreif
Copy link
Contributor Author

ggreif commented Mar 5, 2020

@GrahamcOfBorg build llvmPackages_10 llvmPackages_10.clang-polly-unwrapped llvmPackages_10.clang-unwrapped

@ggreif
Copy link
Contributor Author

ggreif commented Mar 6, 2020

Local build on Darwin nix-build -A llvmPackages_10 gives

Scanning dependencies of target clangTidyCppCoreGuidelinesModule
[ 93%] Built target clangTidyAbseilModule
[ 93%] Linking CXX static library ../../../../lib/libclangTidyCppCoreGuidelinesModule.a
[ 93%] Built target clangTidyFuchsiaModule
[ 93%] Built target clangTidyCppCoreGuidelinesModule
Scanning dependencies of target clangTidyBugproneModule
Undefined symbols for architecture x86_64:
  "getPollyPluginInfo()", referenced from:
[ 93%] Linking CXX static library ../../../../lib/libclangTidyBugproneModule.a
      (anonymous namespace)::EmitAssemblyHelper::EmitAssemblyWithNewPassManager(clang::BackendAction, std::__1::unique_ptr<llvm::raw_pwrite_stream, std::__1::default_delete<llvm::raw_pwrite_stream> >) in BackendUtil.cpp.o
ld: symbol(s) not found for architecture x86_64
clang-7: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [tools/clang-shlib/CMakeFiles/clang-cpp.dir/build.make:1685: lib/libclang-cpp.dylib] Error 1
make[1]: *** [CMakeFiles/Makefile2:10545: tools/clang-shlib/CMakeFiles/clang-cpp.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 93%] Built target clangTidyBugproneModule
make: *** [Makefile:130: all] Error 2
builder for '/nix/store/qgaq87wflydxnzbs7r8x1l0dnw9bqz8y-clang-10.0.0rc3.drv' failed with exit code 2
error: build of '/nix/store/qgaq87wflydxnzbs7r8x1l0dnw9bqz8y-clang-10.0.0rc3.drv' failed

@DieGoldeneEnte maybe you can confirm this build error on Linux too? I am suspecting a linker bug.

Or maybe it's not a linker bug, as I see:

CMake Warning:
  Manually-specified variables were not used by the project:

    BUILD_TESTING
    CMAKE_EXPORT_NO_PACKAGE_REGISTRY
    LINK_POLLY_INTO_TOOLS
    WITH_POLLY

Do we activate Polly the wrong way? Did the options change? Or is polly on by default now?

Relevant: http://prereleases.llvm.org/10.0.0/rc3/tools/polly/docs/ReleaseNotes.html

@ggreif
Copy link
Contributor Author

ggreif commented Mar 6, 2020

@Meinersbur may I reach out to you because of the above build error? llvm/llvm-project@d7afdb5 looks like a relevant commit for debugging this, but I am not knowledgeable enough about its mechanics. The major question is whether Polly is in clang by default and how it should be activated via cmake if not.

@DieGoldeneEnte
Copy link
Contributor

I can confirm the same error on linux.
If I understand it correctly we need to enable polly with LLVM_ENABLE_PROJECTS, but I only had a quick look.

@Meinersbur
Copy link

Meinersbur commented Mar 6, 2020

I don't now what nix-build does. We cherry-picked into rc3 that LLVM_POLLY_LINK_INTO_TOOLS defaults to ON when polly is in LLVM_ENABLE_PROJECTS. LINK_POLLY_INTO_TOOLS and WITH_POLLY was internal before and should not have been set directly. After https://reviews.llvm.org/D61446, they do not exist anymore.

What cmake variables does nix-build set? I suggest to clean them up. Does the nix package apply any additional patches (like debian)?

@Meinersbur
Copy link

The major question is whether Polly is in clang by default and how it should be activated via cmake if not.

Polly is built in the all target with LLVM_ENABLE_PROJECTS=polly or LLVM_ENABLE_PROJECTS=all. It is also added to clang (and opt/bugoint) if LLVM_POLLY_LINK_INTO_TOOLS=ON, by creating a dependency to getPollyPluginInfo.

There were bugs with dylibs before. See llvm/llvm-project#120

@Meinersbur
Copy link

Meinersbur commented Mar 6, 2020

The fixes llvm/llvm-project@3a0f6e6 and llvm/llvm-project@87dac7d are not cherry-picked into clang 10.0 because of the risk so short before the release. They should be in clang 10.0.1.

If your package manager supports it, you could apply the patches locally.

@ggreif
Copy link
Contributor Author

ggreif commented Mar 7, 2020

@Meinersbur Thanks for the hints! I have included the three relevant commits as patches to llvm and clang, and now I see (in the polly-flavoured llvm):

[nix-shell:~/nixpkgs]$ cat /nix/store/l8ij9nzph5m6pl0dr9zn3k22p4zaqgzi-llvm-10.0.0rc3/lib/cmake/llvm/LLVMConfigExtensions.cmake
set(LLVM_STATIC_EXTENSIONS Polly)

How can I test whether the corresponding clang also picks up the polly-flavour?

Looks like we are fine:

[nix-shell:~/nixpkgs]$ $(nix-build -A llvmPackages_10.clang-polly-unwrapped)/bin/clang -O3 -mllvm -polly -c file.c

[nix-shell:~/nixpkgs]$ $(nix-build -A llvmPackages_10.clang-unwrapped)/bin/clang -O3 -mllvm -polly -c file.c
clang (LLVM option parsing): Unknown command line argument '-polly'.  Try: 'clang (LLVM option parsing) --help'
clang (LLVM option parsing): Did you mean '--color'?

The non-polly flavour doesn't understand -polly, while the former, based on llvm-polly does. In the llvm-polly build, we simply add polly as a subdirectory into $sourceRoot/tools. The clang build then picks it up from there via cmake config file. No LLVM_ENABLE_PROJECTS=polly (or such) flags are needed.

http://lists.llvm.org/pipermail/llvm-dev/2020-March/139729.html

Additionally cherry-picked 3 commits from `llvm-project/master`:
- llvm/llvm-project@d21664c
- llvm/llvm-project@3a0f6e6
- llvm/llvm-project@87dac7d

such that clang can automatically pick up the polly plugin from the
`llvm-polly` build.
@ggreif
Copy link
Contributor Author

ggreif commented Mar 7, 2020

@DieGoldeneEnte PTAL, I think this is ready for merging. CC @basvandijk

@Meinersbur
Copy link

Meinersbur commented Mar 7, 2020

The non-polly flavour doesn't understand -polly, while the former, based on llvm-polly does. In the llvm-polly build, we simply add polly as a subdirectory into $sourceRoot/tools. The clang build then picks it up from there via cmake config file. No LLVM_ENABLE_PROJECTS=polly (or such) flags are needed.

In course of moving to the git monorepository, LLVM changed the project layout from polly in the llvm/tools subdirectory to polly in a sibling directory of llvm, i.e. a top-level directory in the monorepository (like all the other LLVM subprojects). The mechanisms are slightly different. While before the monorepository, you could assume that if somebody explicitly checked-out polly they also want to activate it (i.e. the project is enabled by default of the repository is present), with the monorepository nearly everyone has it checked out (To explicitly enable it, use LLVM_ENABLE_PROJECTS=polly).

Since the the developers, including me, are using the monorepository layout, the subproject layout will be less-and-less tested, and I expect it to be removed at some point to simplify the build system.

@DieGoldeneEnte
Copy link
Contributor

@ggreif LGTM. It compiles without problems and polly is only available for the polly-variant.
Moving to the monorepository layout is in my opinion a good idea, but out of scope for this change.

@ggreif
Copy link
Contributor Author

ggreif commented Mar 8, 2020

Moving to the monorepository layout is in my opinion a good idea, but out of scope for this change.

Dito. Let's iterate.

@ggreif
Copy link
Contributor Author

ggreif commented Mar 10, 2020

Ping @dtzWill @matthewbauer

@basvandijk
Copy link
Member

@GrahamcOfBorg build llvmPackages_10 llvmPackages_10.clang-polly-unwrapped llvmPackages_10.clang-unwrapped

@basvandijk basvandijk merged commit 479da57 into NixOS:master Mar 11, 2020
@ggreif ggreif deleted the llvm-10 branch March 11, 2020 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants