Re-enable nvptx tests #96842
Closed
Re-enable nvptx tests #96842
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
@rustbot label +O-NVPTX |
rustbot
added
the
O-NVPTX
nagisa
added
S-blocked
|
Revisiting this since the last time, my memory is that this is still blocked on getting a release of |
|
I agree with the above. As a preliminary to forking it to a more official place I opened an issue asking for re-licensing into MIT/Apache-2 dual license. I am a bit confused about to what degree this is required? It is simple enough to just fork it and add the release for a quick fix of the CI. But I'm also under the impression this might make it more messy if done before the re-licensing (assumed it is required). If anyone have any insight on how important the re-licensing is I can attempt to reach out to @denzp using a different channel. It is simpler for me to request something concrete if I can point to it being necessary for tighter integration. From old comments from @denzp it seemed like his motivation was to make ptx-linker available through an official channel down the road, and I hope he is still willing to make it happen. It seems like his activity on github is zero for many months so I assume he just haven't noticed my issue. |
|
cc @pnkfelix @wesleywiser license dragons be ↑here↑. |
14 tasks
|
@kjetilkjeka @rustbot label: +S-inactive |
rustbot
added
the
S-inactive
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Mar 11, 2024
…rochenkov LLVM Bitcode Linker: A self contained linker for nvptx and other targets This PR introduces a new linker named `llvm-bitcode-linker`. It is a `self-contained` linker that can be used to link programs in `llbc` before optimizing and compiling to native code. It will first be used internally in the Rust compiler to enable tests for the `nvptx64-nvidia-cuda` target as the original `rust-ptx-linker` is deprecated. It will then be provided to users of the `nvptx64-nvidia-cuda` target with the purpose of linking ptx. More targets than nvptx will also be supported eventually. The PR introduces a new unstable `LinkerFlavor` for the compiler. The compiler will also not be shipped with rustc but most likely instead be shipped in it's own unstable component (a follow up PR will be opened for this). This means that merging this PR should not add any stability guarantees. When more details of `self-contained` is implemented it will only be possible to use the linker when `-Clink-self-contained=+linker` is passed. <details> <summary>Original Description</summary> **When this PR was created it was focused a bit differently. The original text is preserved here in case there's some interests in it** I have experimenting with approaches to replace the ptx-linker and enable the nvptx target tests again. I think it's time to get some feedback on the approach. ### The problem The only useful linker for the nvptx target is [this crate](https://github.com/denzp/rust-ptx-linker). Since this linker performs linking on llvm bitcode it needs to track the llvm version of rustc and use the same format. It has not been maintained for 3+ years and must be considered abandoned. Over the years rust have upgraded LLVM while the linker has been left to bitrot. It is no longer in a usable state. Due to the difficulty of keeping the ptx-linker up to date outside of tree the nvptx tests was [disabled a long time ago](rust-lang@f8f9a28). It was [previously discussed](rust-lang#96842 (comment)) if adding the ptx-linker to the rust repo would be a possibility. My efforts in doing this stopped at getting an answered if the license would prohibit it from inclusion in the [Rust repo](rust-lang#96842 (comment)). I therefore concluded that a re-write would be necessary. ### The possible solution presented here The llvm tools know perfectly well how to link and optimize llvm bitcode. Each of them only perform a single task, and are therefore a bit cumbersome to call with the current linker approach rustc takes. This PR adds a simple tool (current name `embedded-linker`) which can link self contained (often embedded) programs in llvm bitcode before compiling to the target format. Optimization will also be performed if lto is enabled. The rust compiler will make a single invocation to this tool, while the tool will orchestrate the many calls to the llvm tools. ### The questions - Is having control over the nvptx linking and therefore also tests worth it to add such tool? or should the tool live outside the rust repo? - Is the approach of calling llvm tools acceptable? Or would we want to keep the ptx-linker approach of using the llvm library? The tools seems to provide more simplicity and stability, but more intermediate files are being written. Perhaps there also are some performance penalty for the calling tools approach. - What is the process for adding such tool? MCP? - Does adding `llvm-link` to the llvm-tool component require any process? - Does it require some sort of FCP to remove ptx-linker as the default linker for ptx? Or is it sufficient that using the upstream ptx-linker is broken in its current state. it is possible to use a somewhat patched version of ptx-linker. </details>
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 11, 2024
Rollup merge of rust-lang#117458 - kjetilkjeka:embedded-linker, r=petrochenkov LLVM Bitcode Linker: A self contained linker for nvptx and other targets This PR introduces a new linker named `llvm-bitcode-linker`. It is a `self-contained` linker that can be used to link programs in `llbc` before optimizing and compiling to native code. It will first be used internally in the Rust compiler to enable tests for the `nvptx64-nvidia-cuda` target as the original `rust-ptx-linker` is deprecated. It will then be provided to users of the `nvptx64-nvidia-cuda` target with the purpose of linking ptx. More targets than nvptx will also be supported eventually. The PR introduces a new unstable `LinkerFlavor` for the compiler. The compiler will also not be shipped with rustc but most likely instead be shipped in it's own unstable component (a follow up PR will be opened for this). This means that merging this PR should not add any stability guarantees. When more details of `self-contained` is implemented it will only be possible to use the linker when `-Clink-self-contained=+linker` is passed. <details> <summary>Original Description</summary> **When this PR was created it was focused a bit differently. The original text is preserved here in case there's some interests in it** I have experimenting with approaches to replace the ptx-linker and enable the nvptx target tests again. I think it's time to get some feedback on the approach. ### The problem The only useful linker for the nvptx target is [this crate](https://github.com/denzp/rust-ptx-linker). Since this linker performs linking on llvm bitcode it needs to track the llvm version of rustc and use the same format. It has not been maintained for 3+ years and must be considered abandoned. Over the years rust have upgraded LLVM while the linker has been left to bitrot. It is no longer in a usable state. Due to the difficulty of keeping the ptx-linker up to date outside of tree the nvptx tests was [disabled a long time ago](rust-lang@f8f9a28). It was [previously discussed](rust-lang#96842 (comment)) if adding the ptx-linker to the rust repo would be a possibility. My efforts in doing this stopped at getting an answered if the license would prohibit it from inclusion in the [Rust repo](rust-lang#96842 (comment)). I therefore concluded that a re-write would be necessary. ### The possible solution presented here The llvm tools know perfectly well how to link and optimize llvm bitcode. Each of them only perform a single task, and are therefore a bit cumbersome to call with the current linker approach rustc takes. This PR adds a simple tool (current name `embedded-linker`) which can link self contained (often embedded) programs in llvm bitcode before compiling to the target format. Optimization will also be performed if lto is enabled. The rust compiler will make a single invocation to this tool, while the tool will orchestrate the many calls to the llvm tools. ### The questions - Is having control over the nvptx linking and therefore also tests worth it to add such tool? or should the tool live outside the rust repo? - Is the approach of calling llvm tools acceptable? Or would we want to keep the ptx-linker approach of using the llvm library? The tools seems to provide more simplicity and stability, but more intermediate files are being written. Perhaps there also are some performance penalty for the calling tools approach. - What is the process for adding such tool? MCP? - Does adding `llvm-link` to the llvm-tool component require any process? - Does it require some sort of FCP to remove ptx-linker as the default linker for ptx? Or is it sufficient that using the upstream ptx-linker is broken in its current state. it is possible to use a somewhat patched version of ptx-linker. </details>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a PR to re-enable CI testing of the nvptx64-nvidia-cuda target. It requires two things to work:
ptx-linker was removed from the CI in 2019 here. This was due to the old version relying on symbols from rustc_codegen_llvm. This has been changed since then.
My proposal for the short term is that @denzp creates a release for v0.9.1 on github. Then we can then download it with wget/curl like we used to do with the 0.9.0 ptx linker. This PR is therefore blocked right now on a suitable place where ptx-linker can be downloaded.
For the long term ptx-linker should be distributed in a more proper way, but I suspect this is not the most pressing matter when it comes to nvptx. I have created this issue asking for re-licensing to MIT/Apache-2. But I'm honestly not sure if it is absolutely necessary for a tool? There are also many more things I'm uncertain about when it comes to the long-term plan like: where the code should live, does it need to be a rustup component or can it be bundled with the nvptx64 toolchain, and what the proper way to build it for nvptx targets in CI. Hopefully we do not have to attack that right now before making progress on the CI situation?
Requesting review since nagisa is familiar with the work I'm currently doing from previous PR.
r? @nagisa