Skip to content

Trim and tidy includes in rustc_llvm #132584

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Nov 9, 2024
Merged

Trim and tidy includes in rustc_llvm #132584

merged 2 commits into from
Nov 9, 2024

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Nov 4, 2024

These includes tend to accumulate over time, and are usually only removed when something breaks in a new LLVM version, so it's nice to clean them up manually once in a while.

General strategy used for this PR:

  • Remove all includes from LLVMWrapper.h that aren't needed by the header itself, transplanting them to individual source files as necessary.
  • For each source file, temporarily remove each include if doing so doesn't cause a compile error.
  • If a “required” include looks like it shouldn't be needed, try replacing it with its sub-includes, then trim that list.
  • After doing all of the above, go back and re-add any removed include if the file does actually use things defined in that header, even if the header happens to also be included by something else.

@rustbot
Copy link
Collaborator

rustbot commented Nov 4, 2024

r? @cuviper

rustbot has assigned @cuviper.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Nov 4, 2024
@jieyouxu jieyouxu added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Nov 4, 2024
@cuviper
Copy link
Member

cuviper commented Nov 8, 2024

LGTM, as long as it does compile. 😉

@bors r+ rollup=iffy

@bors
Copy link
Collaborator

bors commented Nov 8, 2024

📌 Commit 920d277 has been approved by cuviper

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 Nov 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2024
Trim and tidy includes in `rustc_llvm`

These includes tend to accumulate over time, and are usually only removed when something breaks in a new LLVM version, so it's nice to clean them up manually once in a while.

General strategy used for this PR:
- Remove all includes from `LLVMWrapper.h` that aren't needed by the header itself, transplanting them to individual source files as necessary.
- For each source file, temporarily remove each include if doing so doesn't cause a compile error.
- If a “required” include looks like it shouldn't be needed, try replacing it with its sub-includes, then trim that list.
- After doing all of the above, go back and re-add any removed include if the file does actually use things defined in that header, even if the header happens to also be included by something else.
@bors
Copy link
Collaborator

bors commented Nov 9, 2024

⌛ Testing commit 920d277 with merge 6ddea26...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-mingw failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
test [run-make] tests\run-make\zero-extend-abi-param-passing ... ok
test [run-make] tests\run-make\test-benches ... ok
test [run-make] tests\run-make\share-generics-export-again ... ok
test [run-make] tests\run-make\rustc-crates-on-stable ... ok
Terminate batch job (Y/N)? 
##[error]The operation was canceled.
Post job cleanup.
[command]"C:\Program Files\Git\bin\git.exe" version
git version 2.47.0.windows.1

@bors
Copy link
Collaborator

bors commented Nov 9, 2024

💔 Test failed - checks-actions

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

Zalathar commented Nov 9, 2024

@bors retry (unrelated timeout)

@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 Nov 9, 2024
@bors
Copy link
Collaborator

bors commented Nov 9, 2024

⌛ Testing commit 920d277 with merge 4b198d6...

@bors
Copy link
Collaborator

bors commented Nov 9, 2024

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing 4b198d6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 9, 2024
@bors bors merged commit 4b198d6 into rust-lang:master Nov 9, 2024
7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 9, 2024
@Zalathar Zalathar deleted the includes branch November 9, 2024 12:24
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4b198d6): 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 (secondary -2.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)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-3.5%, -2.3%] 2
All ❌✅ (primary) - - 0

Cycles

Results (secondary -2.6%)

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
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.6% [-3.1%, -2.3%] 4
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 781.973s -> 780.833s (-0.15%)
Artifact size: 335.37 MiB -> 335.34 MiB (-0.01%)

mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
Trim and tidy includes in `rustc_llvm`

These includes tend to accumulate over time, and are usually only removed when something breaks in a new LLVM version, so it's nice to clean them up manually once in a while.

General strategy used for this PR:
- Remove all includes from `LLVMWrapper.h` that aren't needed by the header itself, transplanting them to individual source files as necessary.
- For each source file, temporarily remove each include if doing so doesn't cause a compile error.
- If a “required” include looks like it shouldn't be needed, try replacing it with its sub-includes, then trim that list.
- After doing all of the above, go back and re-add any removed include if the file does actually use things defined in that header, even if the header happens to also be included by something else.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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
Development

Successfully merging this pull request may close these issues.

7 participants