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

Const prop: erase all block-only locals at the end of every block #73757

Merged
merged 1 commit into from
Jun 28, 2020

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 26, 2020

I messed up this erasure in #73656 (comment). I think it is too fragile to have the previous scheme. Let's benchmark the new scheme and see what happens.

r? @wesleywiser

cc @felix91gr

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 26, 2020
@oli-obk oli-obk changed the title Const prop hardening Const prop: erase all block-only locals at the end of every block Jun 26, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 26, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jun 26, 2020

⌛ Trying commit 783021cc5ce3268672dbcd67afd91ab61b044cd9 with merge 2d4319b8222eaaca3cec82a0c9c18f3c86284b1c...

@wesleywiser
Copy link
Member

Can we add a regression test for this?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 26, 2020

The other PR has a bug reproducing the issue. Do you want me to move it here? It doesn't do anything here (and it doesn't do anything anymore on the other PR). It's just a piece of code that const prop has no business of ever "improving".

@wesleywiser
Copy link
Member

Oh, I see. It only triggers with the deaggregator move up. That's fine then.

@wesleywiser
Copy link
Member

r=me if the perf looks ok

@bors
Copy link
Contributor

bors commented Jun 26, 2020

☀️ Try build successful - checks-azure
Build commit: 2d4319b8222eaaca3cec82a0c9c18f3c86284b1c (2d4319b8222eaaca3cec82a0c9c18f3c86284b1c)

@rust-timer
Copy link
Collaborator

Queued 2d4319b8222eaaca3cec82a0c9c18f3c86284b1c with parent 14e65d5, future comparison URL.

@felix91gr
Copy link
Contributor

Sees iter_enumerate

@oli-obk you're a genius.

Let's see how the perf turns out, this should definitely be better :3

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (2d4319b8222eaaca3cec82a0c9c18f3c86284b1c): comparison url.

@felix91gr
Copy link
Contributor

Interesting change. Are we sure we're comparing the right commits? @oli-obk

@felix91gr
Copy link
Contributor

Fwiw, here it is v/s its direct parent: https://perf.rust-lang.org/compare.html?start=e093b6525079cb71d4158f97480ac6f6ce311eac&end=2d4319b8222eaaca3cec82a0c9c18f3c86284b1c&stat=instructions%3Au

What a weird change. I don't get it. Is enumerate that much costlier?

@wesleywiser
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jun 27, 2020

⌛ Trying commit 94b17b74fe809430c5d34f0859a16de474272d8d with merge cd6bfbcb5627d1fba2f5b200a9cf88e52c0aca41...

@@ -334,6 +334,13 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
)
.expect("failed to push initial stack frame");

let mut only_propagate_inside_block_locals = BitSet::new_empty(can_const_prop.len());
for (l, mode) in can_const_prop.iter_enumerated() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh! I don't think I realized that doing this construction here was legal. This is nice :)

So, just to check: this code is executed once per block, right? @wesleywiser

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise, I don't think this change is 100% sound.
Around line 1082, we might be removing consts from the const pool that are not from the current block, yet are restricted to propagate only into their own block. If that makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it passed the tests, so either it's sound, or it threaded the needle through our tests.
Still I think that the failure mode of this expansion to const-prop (if it is buggy) is to propagate less, and not propagate more constants nor the wrong constants. So even if it's buggy, it shouldn't be actually unsound.
To catch those cases we'll need more tests. I hope I can work on that after I finish getting through bureaucracy with uni >.<

@bors
Copy link
Contributor

bors commented Jun 27, 2020

☀️ Try build successful - checks-azure
Build commit: cd6bfbcb5627d1fba2f5b200a9cf88e52c0aca41 (cd6bfbcb5627d1fba2f5b200a9cf88e52c0aca41)

@rust-timer
Copy link
Collaborator

Queued cd6bfbcb5627d1fba2f5b200a9cf88e52c0aca41 with parent 7750c3d, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (cd6bfbcb5627d1fba2f5b200a9cf88e52c0aca41): comparison url.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 27, 2020

Better, but still a regression if there are many blocks

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 27, 2020

I'm working on this

@wesleywiser
Copy link
Member

Looks like we're ready for another round of benchmarks

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jun 27, 2020

⌛ Trying commit 70fdfe2244da7183ff81b0d25c413bcbce92e672 with merge c89495d665db21d0da36eaaec48938e506760d65...

@bors
Copy link
Contributor

bors commented Jun 27, 2020

☀️ Try build successful - checks-azure
Build commit: c89495d665db21d0da36eaaec48938e506760d65 (c89495d665db21d0da36eaaec48938e506760d65)

@rust-timer
Copy link
Collaborator

Queued c89495d665db21d0da36eaaec48938e506760d65 with parent 394e1b4, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (c89495d665db21d0da36eaaec48938e506760d65): comparison url.

Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

r=me on the approach since the perf looks good. Feel free to squash everything together if you want or we can also just merge as-is.

/// Since writing a ZST is not actually accessing memory or locals, this is never invoked
/// for ZST reads.
#[inline]
fn access_local_mut<'a>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @RalfJung some miri engine and machine extensions happen in this PR. They don't change anything, they just allow the const propagator to hook into accessing a local mutably and run some extra code.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the heads-up!

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 28, 2020

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Jun 28, 2020

📌 Commit b7801df1c6814ce748b3a179dc06f69756cabba8 has been approved by wesleywiser

@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 28, 2020
@@ -192,6 +192,8 @@ pub trait Machine<'mir, 'tcx>: Sized {
) -> InterpResult<'tcx>;

/// Called to read the specified `local` from the `frame`.
/// Since reading a ZST is not actually accessing memory or locals, this is never invoked
/// for ZST reads.
Copy link
Member

Choose a reason for hiding this comment

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

To be clear, this was already the case before but now is documented better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@@ -740,7 +740,7 @@ where
// but not factored as a separate function.
let mplace = match dest.place {
Place::Local { frame, local } => {
match self.stack_mut()[frame].locals[local].access_mut()? {
match M::access_local_mut(self, frame, local)? {
Copy link
Member

Choose a reason for hiding this comment

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

There's a risk here of accidentally using the frame method instead of the hook. Is there anything we can do about that?

I think there should at least be a comment at the frame method saying to not call it (except when implementing the hook).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll investigate whether we can have something static and otherwise document it properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I documented it. I don't think it's realistic to statically prevent it being called except by introducing some appropriately named token types.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 28, 2020

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Jun 28, 2020

📌 Commit b9f4e0d has been approved by wesleywiser

@@ -131,6 +131,10 @@ pub enum LocalValue<Tag = ()> {
}

impl<'tcx, Tag: Copy + 'static> LocalState<'tcx, Tag> {
/// Read the local's value or error if the local is not yet live or not live anymore.
///
/// Note: This may only be invoked from the `Machine::access_local` hook and not from
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Note: This may only be invoked from the `Machine::access_local` hook and not from
/// Note: This must only be invoked from the `Machine::access_local` hook and not from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had must before 😆 I think may is correct here.

@@ -143,6 +147,9 @@ impl<'tcx, Tag: Copy + 'static> LocalState<'tcx, Tag> {

/// Overwrite the local. If the local can be overwritten in place, return a reference
/// to do so; otherwise return the `MemPlace` to consult instead.
///
/// Note: This may only be invoked from the `Machine::access_local_mut` hook and not from
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Note: This may only be invoked from the `Machine::access_local_mut` hook and not from
/// Note: This must only be invoked from the `Machine::access_local_mut` hook and not from

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 28, 2020
…arth

Rollup of 9 pull requests

Successful merges:

 - rust-lang#73577 (Add partition_point)
 - rust-lang#73757 (Const prop: erase all block-only locals at the end of every block)
 - rust-lang#73774 (Make liveness more precise for assignments to fields)
 - rust-lang#73795 (Add some `const_compare_raw_pointers`-related regression tests)
 - rust-lang#73800 (Forward Hash::write_iN to Hash::write_uN)
 - rust-lang#73813 (Rename two `Resolver` traits)
 - rust-lang#73817 (Rename clashing_extern_decl to clashing_extern_declarations.)
 - rust-lang#73826 (Fix docstring typo)
 - rust-lang#73833 (Remove GlobalCtxt::enter_local)

Failed merges:

r? @ghost
@bors bors merged commit ccc1bf7 into rust-lang:master Jun 28, 2020
@oli-obk oli-obk deleted the const_prop_hardening branch March 16, 2021 12:14
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants