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.

Sign up for GitHub

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

Jump to bottom

rustc_codegen_llvm cleanups #130506

Merged
merged 14 commits into from
Sep 20, 2024
Merged

rustc_codegen_llvm cleanups #130506

merged 14 commits into from
Sep 20, 2024

Conversation

nnethercote
Copy link
Contributor

Some improvements I found while reading through this crate's code.

r? @michaelwoerister

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 18, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 18, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in coverage instrumentation.

cc @Zalathar

jieyouxu
jieyouxu approved these changes Sep 18, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT the changes look good, and they do make the code nicer to read 👍 Some of the changes are a bit more subtle (like the hidden visibility logic), so I would like someone else to also take a look.

compiler/rustc_codegen_llvm/src/builder.rs
Comment on lines +320 to +315
set_math_builder_methods! {
fadd_fast(x, y) => (LLVMBuildFAdd, LLVMRustSetFastMath),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remark: I do like how these look 👍

@bors
Copy link
Contributor

bors commented Sep 18, 2024

☔ The latest upstream changes (presumably #130519) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote nnethercote force-pushed the rustc_codegen_llvm-cleanups branch from 9f62143 to e9582bb Compare September 18, 2024 23:25
@nnethercote
Copy link
Contributor Author

The hidden visibility commit is definitely the trickiest one! I have fixed the conflicts and updated.

@bors
Copy link
Contributor

bors commented Sep 19, 2024

☔ The latest upstream changes (presumably #130534) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote
Streamline register methods.
785a26a 
These can be made more concise, mostly through appropriate use of `use`
declarations.
@nnethercote
Rename a parameter.
63210bd 
This seems to be a typo. `singletree` doesn't make sense, and everywhere
else it is `singlethread`.
@nnethercote
Streamline report_inline_asm.
9429e64 
By using `use`.
@nnethercote
Reduce repetition in target_is_apple.
0d78f1e
@nnethercote
Use a macro to factor out some repetitive code.
4ce010e 
Similar to the existing macro just above.
@nnethercote
Remove a low-value comment.
eb57550 
We rarely use parameter comments, and these ones don't tell us anything
interesting.
@nnethercote
Streamline hidden visibility setting.
fda530d 
In `get_fn` there is a complicated set of if/elses to determine if
`hidden` visibility should be applied. There are five calls to
`LLVMRustSetVisibility` and some repetition in the comments.

This commit streamlines it a bit:
- Computes `hidden` and then uses it to determine if a single call to
  `LLVMRustSetVisibility` occurs.
- Converts some of the if/elses into boolean expressions.
- Removes the repetitive comments.

Overall this makes it quite a bit shorter, and I find it easier to read.
@nnethercote
Reorder ConstMethods.
bfef261 
It's crazy to have the integer methods in something close to random
order.

The reordering makes the gaps clear: `const_i64`, `const_i128`,
`const_isize`, and `const_u16`. I guess they just aren't needed.
@nnethercote
Reduce visibility.
badd8cc
@nnethercote
Fix a comment.
ccd6c61 
I'm pretty sure `CodegenCx` applies to codegen units, rather than
compilation units.
@nnethercote
Remove a low-value local variable.
3b07169
@nnethercote
Avoid heavy repetition in llvm/ffi.rs.
c5af8b2 
Through judicious use of `use` and `Self`.
@nnethercote
Merge adjacent unsafe extern "C" blocks.
5fd16df
@nnethercote
Reformat some comments.
1f35940 
So they are less than 100 chars.
@nnethercote nnethercote force-pushed the rustc_codegen_llvm-cleanups branch from e9582bb to 1f35940 Compare September 19, 2024 10:17
jieyouxu
jieyouxu approved these changes Sep 19, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I looked at the hidden visibility changes again, and I think the restructure is correct (as in I think it does preserve the original logic). Please r=me after PR CI is green.

@nnethercote
Copy link
Contributor Author

@bors r=jieyouxu

@bors
Copy link
Contributor

bors commented Sep 20, 2024

📌 Commit 1f35940 has been approved by jieyouxu

It is now in the queue for this repository.

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

bors commented Sep 20, 2024

⌛ Testing commit 1f35940 with merge 1a5a224...

@bors
Copy link
Contributor

bors commented Sep 20, 2024

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 1a5a224 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 20, 2024
@bors bors merged commit 1a5a224 into rust-lang:master Sep 20, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 20, 2024
This was referenced Sep 20, 2024
Open
Open
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1a5a224): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary 3.9%)

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)		 3.9% [3.9%, 3.9%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 3.9% [3.9%, 3.9%] 1

Cycles

Results (primary 3.9%)

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)		 3.9% [3.9%, 3.9%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 3.9% [3.9%, 3.9%] 1

Binary size

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

Bootstrap: 769.323s -> 768.108s (-0.16%)
Artifact size: 341.26 MiB -> 341.28 MiB (0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.83.0
Development

Successfully merging this pull request may close these issues.
6 participants
@nnethercote @rustbot @bors @rust-timer @jieyouxu @michaelwoerister