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

ci: docker: dist-various-1: Include RISC-V C compilers #117654

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

alistair23
Copy link
Contributor

The compiler-builtins for RISC-V are missing some key functions, such as __bswapsi2 [1].

We can't just pull in the LLVM compiler-rt builtins as the rust-lang/rust distribution container doesn't have a C compiler [2].

This patch adds RISC-V C compilers to the CI Dockerfile as the first step towards enabling LLVM compiler-rt builtins for RISC-V Rust.

1: rust-lang/compiler-builtins#350
2: rust-lang/compiler-builtins@e4f46b9

@rustbot
Copy link
Collaborator

rustbot commented Nov 7, 2023

r? @Kobzol

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Nov 7, 2023
@Kobzol
Copy link
Contributor

Kobzol commented Nov 8, 2023

The actual CI changes look fine to me, but I don't really know anything about RISC-V and Rust's setup of it, so also CCing @Mark-Simulacrum.

@nikic
Copy link
Contributor

nikic commented Nov 8, 2023

Shouldn't implementation for these be added to compiler-builtins instead?

@alistair23
Copy link
Contributor Author

Shouldn't implementation for these be added to compiler-builtins instead?

From what I can tell it is a chicken and egg problem. I can't add these to compiler-builtins as there is no toolchain available. So step 1 is to slowly add RISC-V toolchains, so then I can support this in compiler-builtins

@nikic
Copy link
Contributor

nikic commented Nov 9, 2023

Shouldn't implementation for these be added to compiler-builtins instead?

From what I can tell it is a chicken and egg problem. I can't add these to compiler-builtins as there is no toolchain available. So step 1 is to slowly add RISC-V toolchains, so then I can support this in compiler-builtins

By adding to compiler-builtins I meant implementing them inside compiler-builtins using rust code, not pulling them C implementation from compiler-rt in. But seeing the symbol list in rust-lang/compiler-builtins#350 (comment) I guess that's not feasible. I initially thought this is only about trivial things like __bswapsi2 only, but it seems to also affect float builtins.

@alistair23
Copy link
Contributor Author

By adding to compiler-builtins I meant implementing them inside compiler-builtins using rust code, not pulling them C implementation from compiler-rt in. But seeing the symbol list in rust-lang/compiler-builtins#350 (comment) I guess that's not feasible. I initially thought this is only about trivial things like __bswapsi2 only, but it seems to also affect float builtins.

Ah. Yeah, I there are still a lot missing, which is why the other targets all pull in the LLVM compiler-rt builtins

@alistair23
Copy link
Contributor Author

Any more comments?

@Kobzol
Copy link
Contributor

Kobzol commented Dec 13, 2023

I'm sorry, I should have rerolled sooner, I'd like to hear Mark's opinion (as I said, I'm not familiar with this).

r? @Mark-Simulacrum

@rustbot rustbot assigned Mark-Simulacrum and unassigned Kobzol Dec 13, 2023
The compiler-builtins for RISC-V are missing some key functions, such as
__bswapsi2 [1].

We can't just pull in the LLVM compiler-rt builtins as the rust-lang/rust
distribution container doesn't have a C compiler [2].

This patch adds RISC-V C compilers to the CI Dockerfile as the first
step towards enabling LLVM compiler-rt builtins for RISC-V Rust.

1: rust-lang/compiler-builtins#350
2: rust-lang/compiler-builtins@e4f46b9

Signed-off-by: Alistair Francis <[email protected]>
@alistair23 alistair23 force-pushed the alistair/riscv-docker branch from f64f64d to d66a964 Compare December 17, 2023 22:24
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Dec 19, 2023

📌 Commit d66a964 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2023
@bors
Copy link
Contributor

bors commented Dec 19, 2023

⌛ Testing commit d66a964 with merge b37d43e...

@bors
Copy link
Contributor

bors commented Dec 19, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing b37d43e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 19, 2023
@bors bors merged commit b37d43e into rust-lang:master Dec 19, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 19, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b37d43e): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.3% [0.6%, 1.9%] 2
Regressions ❌
(secondary)
4.1% [4.1%, 4.1%] 1
Improvements ✅
(primary)
-1.5% [-3.5%, -0.5%] 3
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.4% [-3.5%, 1.9%] 5

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [0.4%, 0.4%] 1

Bootstrap: 672.871s -> 671.959s (-0.14%)
Artifact size: 312.47 MiB -> 312.44 MiB (-0.01%)

@alistair23 alistair23 deleted the alistair/riscv-docker branch December 19, 2023 21:33
alistair23 added a commit to alistair23/compiler-builtins that referenced this pull request Dec 19, 2023
Now that rust-lang/rust#117654 has been merged
the rust-lang/rust distribution containers contain RISC-V C compilers.

This means that we can now enable the "c" feature fallback.

Resolves: rust-lang#350
Signed-off-by: Alistair Francis <[email protected]>
alistair23 added a commit to alistair23/compiler-builtins that referenced this pull request Dec 19, 2023
Now that rust-lang/rust#117654 has been merged
the rust-lang/rust distribution containers contain RISC-V C compilers.

This means that we can now enable the "c" feature fallback.

Resolves: rust-lang#350
Signed-off-by: Alistair Francis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants