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

Make tcx.mk_const more permissive wrt kind argument (impl Into) #105012

Merged
merged 6 commits into from
Nov 29, 2022

Conversation

WaffleLapkin
Copy link
Member

r? @oli-obk you've asked for this >:)

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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 28, 2022
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Nov 28, 2022
compiler/rustc_middle/src/ty/consts.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Nov 28, 2022

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

- Accept `impl Into`
- Implement `From<>` for `ConstKind`

Note: this adds a dependency on `derive_more` (MIT license). It allows
to derive a lot of traits (like `From` here) that would be otherwise
tedious to implement.
... `tcx.mk_const` can now be used instead
`mk_const(ty::ConstKind::X(...), ty)` can now be simplified to
`mk_cosnt(...,                   ty)`.

I searched with the following regex: \mk_const\([\n\s]*(ty::)?ConstKind\
I've left `ty::ConstKind::{Bound, Error}` as-is, they seem clearer this
way.
...it's just `mk_const` but without the sparcles
@oli-obk
Copy link
Contributor

oli-obk commented Nov 28, 2022

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 28, 2022
@bors
Copy link
Contributor

bors commented Nov 28, 2022

⌛ Trying commit b44817f with merge 82877d5eeacaaf632388af232976d23d529ca7c2...

@oli-obk
Copy link
Contributor

oli-obk commented Nov 28, 2022

@bors delegate+

r=me with perf clean and the review comment addressed

@bors
Copy link
Contributor

bors commented Nov 28, 2022

✌️ @WaffleLapkin can now approve this pull request

@oli-obk
Copy link
Contributor

oli-obk commented Nov 28, 2022

@bors p=1 bitrotty

@oli-obk
Copy link
Contributor

oli-obk commented Nov 28, 2022

@rust-timer build 82877d5eeacaaf632388af232976d23d529ca7c2

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (82877d5eeacaaf632388af232976d23d529ca7c2): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -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.9% [0.8%, 1.0%] 6
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)
- - 0
Regressions ❌
(secondary)
6.2% [5.0%, 7.4%] 2
Improvements ✅
(primary)
-2.7% [-2.7%, -2.7%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.7% [-2.7%, -2.7%] 1

Cycles

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 29, 2022
@WaffleLapkin
Copy link
Member Author

regressions
no action needed

??

Also, the regression itself looks super weird...

@oli-obk
Copy link
Contributor

oli-obk commented Nov 29, 2022

--------------------------------------------------------------------------------
Ir         
--------------------------------------------------------------------------------
57,155,936  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir           file:function
--------------------------------------------------------------------------------
 58,696,222  ???:<rustc_middle::ty::subst::GenericArg as rustc_middle::ty::fold::TypeFoldable>::try_fold_with::<rustc_infer::infer::resolve::OpportunisticVarResolver>
-49,549,818  ???:<&rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg> as rustc_middle::ty::fold::TypeFoldable>::try_fold_with::<rustc_infer::infer::resolve::OpportunisticVarResolver>
 33,374,397  ???:<rustc_middle::ty::Ty as rustc_middle::ty::fold::TypeSuperFoldable>::super_fold_with::<rustc_infer::infer::resolve::OpportunisticVarResolver>
-32,572,741  ???:rustc_middle::ty::util::fold_list::<rustc_infer::infer::freshen::TypeFreshener, rustc_middle::ty::Ty, <&rustc_middle::ty::list::List<rustc_middle::ty::Ty> as rustc_middle::ty::fold::TypeFoldable>::try_fold_with<rustc_infer::infer::freshen::TypeFreshener>::{closure
 30,287,775  ???:<rustc_middle::ty::Ty as rustc_middle::ty::fold::TypeSuperFoldable>::super_fold_with::<rustc_infer::infer::freshen::TypeFreshener>
 12,076,808  ???:<rustc_infer::infer::ShallowResolver as rustc_middle::ty::fold::TypeFolder>::fold_ty
  5,009,264  ???:<rustc_middle::ty::Ty as rustc_middle::ty::fold::TypeFoldable>::try_fold_with::<rustc_infer::infer::freshen::TypeFreshener>
  3,392,061  ???:<&rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg> as rustc_middle::ty::fold::TypeFoldable>::try_fold_with::<rustc_middle::ty::fold::RegionFolder>
 -2,596,008  ???:<rustc_middle::ty::context::TyCtxt>::intern_substs
 -2,105,899  ???:<rustc_middle::ty::Ty as rustc_middle::ty::fold::TypeFoldable>::try_fold_with::<rustc_middle::ty::fold::RegionFolder>

Looks like something in inference var resolving regressed.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 29, 2022

@bors r+ looking at the graph of the wg-grammar benchmark this seems to be within the regular ups and downs of that benchmark

@bors
Copy link
Contributor

bors commented Nov 29, 2022

📌 Commit a44eb3c has been approved by oli-obk

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 29, 2022
@bors
Copy link
Contributor

bors commented Nov 29, 2022

⌛ Testing commit a44eb3c with merge e0098a5...

@bors
Copy link
Contributor

bors commented Nov 29, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing e0098a5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 29, 2022
@bors bors merged commit e0098a5 into rust-lang:master Nov 29, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 29, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e0098a5): 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

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)
-1.7% [-2.8%, -0.6%] 2
Improvements ✅
(secondary)
-3.4% [-3.4%, -3.4%] 1
All ❌✅ (primary) -1.7% [-2.8%, -0.6%] 2

Cycles

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

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Make `tcx.mk_const` more permissive wrt `kind` argument (`impl Into`)

r? `@oli-obk` you've asked for this >:)
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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.

6 participants