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

Various refactorings to rustc_interface #126834

Merged
merged 7 commits into from
Jun 25, 2024
Merged

Various refactorings to rustc_interface #126834

merged 7 commits into from
Jun 25, 2024

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jun 22, 2024

This should make it easier to move the driver interface away from queries in the future. Many custom drivers call queries like queries.global_ctxt() before they are supposed to be called, breaking some things like certain --print and -Zunpretty options, -Zparse-only and emitting the dep info at the wrong point in time. They are also not actually necessary at all. Passing around the query output manually would avoid recomputation too and would be just as easy. Removing driver queries would also reduce the amount of global mutable state of the compiler. I'm not removing driver queries in this PR to avoid breaking the aforementioned custom drivers.

@rustbot
Copy link
Collaborator

rustbot commented Jun 22, 2024

r? @michaelwoerister

rustbot has assigned @michaelwoerister.
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 Jun 22, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 22, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Jun 22, 2024
petrochenkov
petrochenkov reviewed Jun 22, 2024
View reviewed changes
compiler/rustc_interface/src/passes.rs Outdated
&sess.opts.unstable_opts.crate_attr,
);

// FIXME modify krate.attrs in place once #65860 is a hard error.
Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree with this and would like to see everything expanded as a part of the regular expansion phase (that's why I introduced the current setup in #108221), that will allow to expand crate-level macro attributes correctly.
If some crate attribute has to be used before that, then it is an exception that should be clearly visible, such attributes need to have some restrictions (e.g. cannot be macro-generated).

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Copy link
Member Author

bjorn3 commented Jun 22, 2024

Seems like dep_graph.finish_encoding() needs to be called even if the incr comp session will never be finalized due to a compile error.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Inline write_dep_info query
7332e79
@bjorn3
Ensure run_compiler always aborts on errors
e3ffbbd 
Before if the closure passed to run_compiler emitted an error without
calling abort_if_errors and no diagnostics have been stashed,
run_compiler would return normally as if no error had occured.
@bjorn3
Avoid a couple of unnecessary EarlyDiagCtxt uses
8d1f5b3
@bjorn3
Copy link
Member Author

bjorn3 commented Jun 22, 2024

I can't reproduce the bug locally. I've dropped the offending commit for now.

@michaelwoerister
Copy link
Member

Current state looks good to me. Thanks, @bjorn3!

Let's not put this into a rollup in case it causes unforeseen breakage.
@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jun 24, 2024

📌 Commit 8d1f5b3 has been approved by michaelwoerister

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 Jun 24, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 25, 2024
@bors
Auto merge of rust-lang#126834 - bjorn3:interface_refactor, r=michael…
fa89b04 
…woerister

Various refactorings to rustc_interface

This should make it easier to move the driver interface away from queries in the future. Many custom drivers call queries like `queries.global_ctxt()` before they are supposed to be called, breaking some things like certain `--print` and `-Zunpretty` options, `-Zparse-only` and emitting the dep info at the wrong point in time. They are also not actually necessary at all. Passing around the query output manually would avoid recomputation too and would be just as easy. Removing driver queries would also reduce the amount of global mutable state of the compiler. I'm not removing driver queries in this PR to avoid breaking the aforementioned custom drivers.
@bors
Copy link
Contributor

bors commented Jun 25, 2024

⌛ Testing commit 8d1f5b3 with merge fa89b04...

@rust-log-analyzer
Copy link
Collaborator

The job dist-aarch64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Jun 25, 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 Jun 25, 2024
@bjorn3
Copy link
Member Author

bjorn3 commented Jun 25, 2024

@bors retry spurious network issue

@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 Jun 25, 2024
@bors
Copy link
Contributor

bors commented Jun 25, 2024

⌛ Testing commit 8d1f5b3 with merge c2d2bb3...

@bors
Copy link
Contributor

bors commented Jun 25, 2024

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing c2d2bb3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 25, 2024
@bors bors merged commit c2d2bb3 into rust-lang:master Jun 25, 2024
7 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 25, 2024
@bjorn3 bjorn3 deleted the interface_refactor branch June 25, 2024 12:51
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c2d2bb3): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)		 0.3% [0.2%, 0.3%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.8% [-5.3%, -0.2%] 2
All ❌✅ (primary) 0.4% [0.4%, 0.4%] 1

Max RSS (memory usage)

Results (primary -4.2%, secondary -3.5%)

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)		 3.9% [3.9%, 3.9%] 1
Improvements ✅
(primary)		 -4.2% [-4.7%, -3.4%] 3
Improvements ✅
(secondary)		 -4.4% [-8.0%, -2.6%] 9
All ❌✅ (primary) -4.2% [-4.7%, -3.4%] 3

Cycles

Results (secondary 2.8%)

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)		 5.3% [2.0%, 8.7%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 693.956s -> 693.71s (-0.04%)
Artifact size: 324.65 MiB -> 326.75 MiB (0.65%)

@bjorn3 bjorn3 mentioned this pull request Jun 30, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 2, 2024
@matthiaskrgr
Rollup merge of rust-lang#127184 - bjorn3:interface_refactor2, r=Nadr…
b0d8e78 
…ieril

More refactorings to rustc_interface

Follow up to rust-lang#126834
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 3, 2024
@matthiaskrgr
Rollup merge of rust-lang#127184 - bjorn3:interface_refactor2, r=Nadr…
9f8cf64 
…ieril

More refactorings to rustc_interface

Follow up to rust-lang#126834
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 4, 2024
@rust-timer
Unrolled build for rust-lang#127184
0f56949 
Rollup merge of rust-lang#127184 - bjorn3:interface_refactor2, r=Nadrieril

More refactorings to rustc_interface

Follow up to rust-lang#126834
@vetleras vetleras mentioned this pull request Sep 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@michaelwoerister michaelwoerister

Labels
A-run-make Area: port run-make Makefiles to rmake.rs 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
Oxidizing run-make tests
Status: Done
Milestone
1.81.0
Development

Successfully merging this pull request may close these issues.
7 participants
@bjorn3 @rustbot @rust-log-analyzer @michaelwoerister @bors @rust-timer @petrochenkov