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

clang_9: fix compilation of HIP-code #77476

Closed
wants to merge 1 commit into from

Conversation

@DieGoldeneEnte
Copy link
Contributor

DieGoldeneEnte commented Jan 10, 2020

Motivation for this change

When compiling HIP-code clang executes other llvm binaries, such as
llvm-link, llc or opt. Clang expects these binaries in the folder with
the clang-executable (...clang/bin/), this is not true for nix, because
these binaries are in the llvm/bin folder. It is not possible to simply
replace the path of the called library, because then the compilation
with gcc fails. While I personally did not encounter a problem with
clang failing due to calling lld I added it for completeness, this
however means clang also depends on the llvm binutils.

Maybe older clang versions have the same flaw, I did not check this yet since my test setup only works with clang9 at the moment. Also I am not sure whether this is the most elegant solution, since I don't have too much experience with clang or nix.

Things done

I checked for successful compilation using this this setup. Building this was not possible without this change.

  • 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.
When compiling HIP-code clang executes other llvm binaries, such as
llvm-link, llc or opt. Clang expects these binaries in the folder with
the clang-executable (...clang/bin/), this is not true for nix, because
these binaris are in the llvm/bin folder. It is not possible to simply
replace the path of the called library, because then the compilation
with gcc fails. While I personally did not encounter a problem with
clang failing due to calling lld I added it for completeness, this
however means clang also depends on the llvm binutils.
@DieGoldeneEnte DieGoldeneEnte requested a review from matthewbauer as a code owner Jan 10, 2020
@DieGoldeneEnte DieGoldeneEnte changed the title clang: fix compilation of HIP-code clang_9: fix compilation of HIP-code Jan 12, 2020
@matthewbauer

This comment has been minimized.

Copy link
Member

matthewbauer commented Jan 14, 2020

Does this just effect clang 9 or should we do it for other clang versions as well?

sed -i -e 's|SmallString<128> ExecPath(C.getDriver().Dir);|SmallString<128> ExecPath(C.getDriver().Name.find("clang") != std::string::npos ? "${llvm}/bin" : C.getDriver().Dir);|' \
-e 's|SmallString<128> OptPath(C.getDriver().Dir);|SmallString<128> OptPath(C.getDriver().Name.find("clang") != std::string::npos ? "${llvm}/bin" : C.getDriver().Dir);|' \
-e 's|SmallString<128> LlcPath(C.getDriver().Dir);|SmallString<128> LlcPath(C.getDriver().Name.find("clang") != std::string::npos ? "${llvm}/bin" : C.getDriver().Dir);|' \
-e 's|SmallString<128> LldPath(C.getDriver().Dir);|SmallString<128> LldPath(C.getDriver().Name.find("clang") != std::string::npos ? "${bintools}/bin" : C.getDriver().Dir);|' \

This comment has been minimized.

Copy link
@matthewbauer

matthewbauer Jan 14, 2020

Member

binutils only has lld for clang toolchains. We should use the lld package here instead.

@matthewbauer

This comment has been minimized.

Copy link
Member

matthewbauer commented Jan 14, 2020

however means clang also depends on the llvm binutils

Clang already has a runtime dependency on llvm, so this is no problem (or at the worst just a few extra wrappers).

Ideally we could upstream this to LLVM. I think instead of using C.getDriver().Dir here, it would be better to do a find_program in CMake like is done for https://github.com/llvm-mirror/clang/blob/65acf43270ea2894dffa0d0b292b92402f80c8cb/CMakeLists.txt#L91-L92. Perhaps just a ticket for LLVM would do.

@DieGoldeneEnte

This comment has been minimized.

Copy link
Contributor Author

DieGoldeneEnte commented Jan 14, 2020

As far as I found, the commit introducing the code is present since llvm 7.0.0 (and will currently also be present in llvm 10). I haven't tried versions other than 9, because I haven't found good test code for checking.

@DieGoldeneEnte

This comment has been minimized.

Copy link
Contributor Author

DieGoldeneEnte commented Jan 15, 2020

I confirmed the problem with clang 8 and 9, for clang7 I would need some more work to get older versions of the device libraries.
I will close this PR while I try to get this fixed upstream, since these changes will not be needed once this is fixed.

DieGoldeneEnte added a commit to DieGoldeneEnte/nixpkgs that referenced this pull request Jan 22, 2020
This PR aims to fix the same problem as NixOS/nixpkgs PR NixOS#77476; enabling to
compile HIP-code using the packaged clang compiler, by also searching in $PATH
for required binaries.
The change is committed upstream (https://reviews.llvm.org/D72903), but will
not land in the clang versions in nixpkgs (only clang 10+). As such I have
created patches for the affected versions. To compile HIP-code lld is needed,
so I added it to the clang-package.<Paste>
DieGoldeneEnte added a commit to DieGoldeneEnte/nixpkgs that referenced this pull request Jan 22, 2020
This PR aims to fix the same problem as NixOS/nixpkgs PR NixOS#77476; enabling to
compile HIP-code using the packaged clang compiler, by also searching in $PATH
for required binaries.
The change is committed upstream (https://reviews.llvm.org/D72903), but will
not land in the clang versions in nixpkgs (only clang 10+). As such I have
created patches for the affected versions. To compile HIP-code lld is needed,
so I added it to the clang-package.<Paste>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.