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

Replace Body::basic_blocks() with field access #99027

Merged
merged 1 commit into from
Aug 29, 2022

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jul 7, 2022

Since the refactoring in #98930, it is possible to borrow the basic blocks
independently from other parts of MIR by accessing the basic_blocks field
directly.

Replace unnecessary Body::basic_blocks() method with a direct field access,
which has an additional benefit of borrowing the basic blocks only.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 7, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 7, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 7, 2022
@JakobDegen
Copy link
Contributor

It's not super clear to me that we want this. DerefMut is really easy to call accidentally, so this might be a footgun for people intending to use .as_mut_no_invalidate() or whatever it's called. Don't feel super strongly either way though

@pnkfelix
Copy link
Member

pnkfelix commented Jul 7, 2022

I also do not see why we would do this.

(Are accessor methods like .basic_blocks_mut() considered an anti-pattern in Rust fashion today?)

@jackh726
Copy link
Member

jackh726 commented Jul 7, 2022

In fairness, Deref is already implemented. It seems a bit silly to have one but not the other?

@tmiasko
Copy link
Contributor Author

tmiasko commented Jul 7, 2022

As to the question why change this at all, I would like to encourage borrowing individual fields instead of the whole body and reduce the number of different ways to borrow the basic blocks.

I also think that it might be confusing to leave basic_blocks_mut(). If so far you only used basic_blocks_mut() to mutably borrow the basic blocks, you probably wouldn't guess how to say borrow mutably basic_blocks and local_decls both (note that the answer isn't basic_blocks_and_local_decls_mut() because that method is already gone).

@JakobDegen
Copy link
Contributor

@jackh726 yeah, but the important difference is that DerefMut, like the .as_mut() that it calls, has a side-effect of invalidating the CFG cache. That's the only reason that this was ever behind a method anyway. I wouldn't mind having this if that wasn't the case.

@tmiasko I'm on board with getting rid of .basic_blocks_mut(). I just think that .as_mut() suffices and we don't need DerefMut

@tmiasko
Copy link
Contributor Author

tmiasko commented Jul 7, 2022

@JakobDegen informed by this refactoring, I think that if one knows about cache invalidation DerefMut is clear enough. If one isn't aware of cache invalidation aspect yet, then as_mut on its own offers no improvement.

I suspect that to make further improvements toward preserving cache validity we would need to go one step further. Avoid DerefMut, IndexMut (body still implements IndexMut<BasicBlock>), and as_mut. Instead using as_mut_preserves_cfg and as_mut_invalidates_cfg or so.

@JakobDegen
Copy link
Contributor

The thing I'm more worried about is accidental calls from someone intending to use the no-cfg-invalidation stuff

@bors
Copy link
Contributor

bors commented Jul 22, 2022

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

@bors
Copy link
Contributor

bors commented Jul 29, 2022

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

@tmiasko
Copy link
Contributor Author

tmiasko commented Jul 30, 2022

r? rust-lang/mir-opt

@rust-highfive rust-highfive assigned oli-obk and unassigned jackh726 Jul 30, 2022
@bors
Copy link
Contributor

bors commented Aug 4, 2022

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

@bors
Copy link
Contributor

bors commented Aug 9, 2022

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

@oli-obk
Copy link
Contributor

oli-obk commented Aug 24, 2022

Instead using as_mut_preserves_cfg and as_mut_invalidates_cfg or so.

I like this, as it makes it clear that there's something possibly surprising going on

@tmiasko tmiasko changed the title Replace Body::basic_blocks*() with field access Replace Body::basic_blocks() with field access Aug 26, 2022
@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 26, 2022

I limited changes to basic_blocks() only and left out mutable variant for another pull request.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 26, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Aug 26, 2022

📌 Commit b48870b has been approved by oli-obk

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 Aug 26, 2022
@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 Aug 26, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 28, 2022
Replace `Body::basic_blocks()` with field access

Since the refactoring in rust-lang#98930, it is possible to borrow the basic blocks
independently from other parts of MIR by accessing the `basic_blocks` field
directly.

Replace unnecessary `Body::basic_blocks()` method with a direct field access,
which has an additional benefit of borrowing the basic blocks only.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 28, 2022
Replace `Body::basic_blocks()` with field access

Since the refactoring in rust-lang#98930, it is possible to borrow the basic blocks
independently from other parts of MIR by accessing the `basic_blocks` field
directly.

Replace unnecessary `Body::basic_blocks()` method with a direct field access,
which has an additional benefit of borrowing the basic blocks only.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 28, 2022
Replace `Body::basic_blocks()` with field access

Since the refactoring in rust-lang#98930, it is possible to borrow the basic blocks
independently from other parts of MIR by accessing the `basic_blocks` field
directly.

Replace unnecessary `Body::basic_blocks()` method with a direct field access,
which has an additional benefit of borrowing the basic blocks only.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 28, 2022
Replace `Body::basic_blocks()` with field access

Since the refactoring in rust-lang#98930, it is possible to borrow the basic blocks
independently from other parts of MIR by accessing the `basic_blocks` field
directly.

Replace unnecessary `Body::basic_blocks()` method with a direct field access,
which has an additional benefit of borrowing the basic blocks only.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 28, 2022
Replace `Body::basic_blocks()` with field access

Since the refactoring in rust-lang#98930, it is possible to borrow the basic blocks
independently from other parts of MIR by accessing the `basic_blocks` field
directly.

Replace unnecessary `Body::basic_blocks()` method with a direct field access,
which has an additional benefit of borrowing the basic blocks only.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 29, 2022
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#94890 (Support parsing IP addresses from a byte string)
 - rust-lang#96334 (socket `set_mark` addition.)
 - rust-lang#99027 (Replace `Body::basic_blocks()` with field access)
 - rust-lang#100437 (Improve const mismatch `FulfillmentError`)
 - rust-lang#100843 (Migrate part of rustc_infer to session diagnostic)
 - rust-lang#100897 (extra sanity check against consts pointing to mutable memory)
 - rust-lang#100959 (translations: rename warn_ to warning)
 - rust-lang#101111 (Use the declaration's SourceInfo for FnEntry retags, not the outermost)
 - rust-lang#101116 ([rustdoc] Remove Attrs type alias)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d182081 into rust-lang:master Aug 29, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 29, 2022
@tmiasko tmiasko deleted the basic-blocks branch August 29, 2022 12:49
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Oct 23, 2022
Replace `Body::basic_blocks()` with field access

Since the refactoring in rust-lang#98930, it is possible to borrow the basic blocks
independently from other parts of MIR by accessing the `basic_blocks` field
directly.

Replace unnecessary `Body::basic_blocks()` method with a direct field access,
which has an additional benefit of borrowing the basic blocks only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

8 participants