-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Remove interior mutability in mir predecessors cache #64736
Remove interior mutability in mir predecessors cache #64736
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
r? @oli-obk |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
64be10d
to
ef33d75
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #64886) made this pull request unmergeable. Please resolve the merge conflicts. |
c52d128
to
176fcc9
Compare
e549f6f
to
3eaad56
Compare
@bors r=oli-obk |
📌 Commit 3eaad56 has been approved by |
…li-obk Remove interior mutability in mir predecessors cache
☀️ Test successful - checks-azure |
} | ||
|
||
impl BodyCache<'tcx> { | ||
pub fn ensure_predecessors(&mut self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to take self by mut ref.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. Cache
's ensure_predecessors
takes self
by mut ref (https://github.com/rust-lang/rust/pull/64736/files/3eaad564d25402013bdf5591c453c916b49cdf93#diff-f1f20e32c0d3522f2f33135df0f4696bR47), and that requires that self.cache
here below is taken by mut ref. That's only possible if you have &mut self
. Or is there some detail I'm overlooking?
Rustup to rust-lang/rust#64736 cc rust-lang/rust#64736 Fixes #4872 changelog: none
Changes: ```` Normalize custom ICE test Rustup to rust-lang#64736 Use assert_crate_local for a more explicit error Rustup to rust-lang#66789 account for external macro in MISSING_INLINE_IN_PUBLIC_ITEMS lint build(tests/fmt): use shared target dir chore: fix and split some ui tests on 32bit system build: set up build job for i686 targets remove needless my_lint ui test git quiet deploy: cd to out/ before adding files to git Less needless_doctest_main false positives fmt Feed the dog Use rustc_env instead of exec_env for test Make triggering this lint less likely 📎 Use exec_env to set backtrace level and normalize output Update custom ICE function with latest rustc Use Clippy version in ICE message Add custom ICE message that points to Clippy repo Fix master deployment Run update_lints Add projections check to EUV for escape analysis Use infer_ctxt Move use_self to nursery Use `println!` on success instead of `eprintln!` Revert "Disable chalk integration test. Output too large" Remove the old integration-tests.sh script Use rust implementation for integration tests in CI Rust implementation of integration test Don't error on clippy.toml of dependencies Fix categorizations Fix arguments on ExprUseVisitor::new euv moved from middle to typeck cmt_ -> Place build: check if RTIM is not installed make use of Result::map_or trigger string_lit_as_bytes when literal has escapes Remove negative float literal checks. Enable deny-warnings feature everywhere in CI Remove unused debugging feature implemented `as_conversions` lint fixing a typo [comparison_chain] rust-lang#4827 Check `core::cmp::Ord` is implemented add a good example for the approx_const lint Add suggested good cases in docs for lifetimes lint ````
result[tgt].push(bb); | ||
} | ||
#[derive(Clone, Debug, HashStable, RustcEncodable, RustcDecodable, TypeFoldable)] | ||
pub struct BodyCache<'tcx> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be named BodyAndCache
to alleviate confusion.
} | ||
} | ||
|
||
impl Index<BasicBlock> for ReadOnlyBodyCache<'a, 'tcx> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need this, since you have Deref
.
|
||
|
||
impl Deref for ReadOnlyBodyCache<'a, 'tcx> { | ||
type Target = Body<'tcx>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, this is a common mistake for wrappers of references.
The target should be the reference, &'a Body<'tcx>
, so that you can actually get e.g. &'tcx mir::BasicBlockData<'tcx>
when 'a = 'tcx
.
I wonder if this PR added any lifetimes to existing code to work around that (likely not codegen because that was already wrongly using &'a Body<'tcx>
instead of &'tcx Body<'tcx>
).
EDIT: thankfully doesn't seem like it. I will likely fix the Deref
impl in #65947.
fn create_funclets<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( | ||
mir: &'a Body<'tcx>, | ||
fn create_funclets<'a, 'b, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( | ||
mir: &'b Body<'tcx>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just &Body<'tcx>
.
Make `Visitor::visit_body` take a plain `&Body` `ReadOnlyBodyAndCache` has replaced `&Body` in many parts of the code base that don't care about basic block predecessors. This includes the MIR `Visitor` trait, which I suspect resulted in many unnecessary changes in rust-lang#64736. This reverts part of that PR to reduce the number of places where we need to pass a `ReadOnlyBodyAndCache`. In the long term, we should either give `ReadOnlyBodyAndCache` more ergonomic name and replace all uses of `&mir::Body` with it at the cost of carrying an extra pointer everywhere, or use it only in places that actually need access to the predecessor cache. Perhaps there is an even nicer alternative. r? @Nashenas88
Changes: ```` Normalize custom ICE test Rustup to rust-lang/rust#64736 Use assert_crate_local for a more explicit error Rustup to rust-lang/rust#66789 account for external macro in MISSING_INLINE_IN_PUBLIC_ITEMS lint build(tests/fmt): use shared target dir chore: fix and split some ui tests on 32bit system build: set up build job for i686 targets remove needless my_lint ui test git quiet deploy: cd to out/ before adding files to git Less needless_doctest_main false positives fmt Feed the dog Use rustc_env instead of exec_env for test Make triggering this lint less likely 📎 Use exec_env to set backtrace level and normalize output Update custom ICE function with latest rustc Use Clippy version in ICE message Add custom ICE message that points to Clippy repo Fix master deployment Run update_lints Add projections check to EUV for escape analysis Use infer_ctxt Move use_self to nursery Use `println!` on success instead of `eprintln!` Revert "Disable chalk integration test. Output too large" Remove the old integration-tests.sh script Use rust implementation for integration tests in CI Rust implementation of integration test Don't error on clippy.toml of dependencies Fix categorizations Fix arguments on ExprUseVisitor::new euv moved from middle to typeck cmt_ -> Place build: check if RTIM is not installed make use of Result::map_or trigger string_lit_as_bytes when literal has escapes Remove negative float literal checks. Enable deny-warnings feature everywhere in CI Remove unused debugging feature implemented `as_conversions` lint fixing a typo [comparison_chain] rust-lang#4827 Check `core::cmp::Ord` is implemented add a good example for the approx_const lint Add suggested good cases in docs for lifetimes lint ````
No description provided.