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

[WIP] rustc_mir: double-check const-promotion candidates for sanity. #63812

Open
wants to merge 1 commit into
base: master
from

Conversation

@eddyb
Copy link
Member

commented Aug 22, 2019

Previously, const promotion involved tracking information about the value in a MIR local (or any part of the computation leading up to that value), aka "qualifs", in a quite stateful manner, which is hard to extend to arbitrary CFGs without a dataflow pass.

However, the nature of the promotion we do is that it's effectively an SSA-like "tree" (or DAG, really), of assigned-once locals - which is how we can take them from the original MIR in the first place.
This structure means that the subset of the MIR responsible for computing any given part of a const-promoted value is readily analyzable by walking that tree/DAG.

This PR implements such an analysis in promote_consts, reusing the HasMutInterior / NeedsDrop computation from qualify_consts, but reimplementing the equivalent of IsNotPromotable / IsNotImplicitlyPromotable.

Eventually we should be able to remove IsNotPromotable / IsNotImplicitlyPromotable from qualify_consts, which will simplify @ecstatic-morse's dataflow-based const-checking efforts.

But currently this is mainly for a crater check-only run - it will compare the results from the old promotion collection and the new promotion validation and ICE if they don't match.

r? @oli-obk

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

⌛️ Trying commit 0229fb0 with merge 28d741e...

bors added a commit that referenced this pull request Aug 22, 2019
Auto merge of #63812 - eddyb:promo-sanity, r=<try>
[WIP] rustc_mir: double-check const-promotion candidates for sanity.

Previously, const promotion involved tracking information about the value in a MIR local (or any part of the computation leading up to that value), aka "qualifs", in a quite stateful manner, which is hard to extend to arbitrary CFGs without a dataflow pass.

However, the nature of the promotion we do is that it's effectively an SSA-like "tree" (or DAG, really), of assigned-once locals - which is how we can take them from the original MIR in the first place.
This structure means that the subset of the MIR responsible for computing any given part of a const-promoted value is readily analyzable by walking that tree/DAG.

This PR implements such an analysis in `promote_consts`, reusing the `HasMutInterior` / `NeedsDrop` computation from `qualify_consts`, but reimplementing the equivalent of `IsNotPromotable` / `IsNotImplicitlyPromotable`.

Eventually we should be able to remove `IsNotPromotable` / `IsNotImplicitlyPromotable` from `qualify_consts`, which will simplify @ecstatic-morse's dataflow-based const-checking efforts.

But currently this is mainly for a crater check-only run - it will compare the results from the old promotion collection and the new promotion validation and ICE if they don't match.

r? @oli-obk
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

☀️ Try build successful - checks-azure
Build commit: 28d741e

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

@craterbot run mode=check-only

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Aug 23, 2019

👌 Experiment pr-63812 created and queued.
🤖 Automatically detected try build 28d741e
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

@rust-timer

This comment has been minimized.

Copy link

commented Aug 23, 2019

Success: Queued 28d741e with parent 201e52e, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented Aug 23, 2019

Finished benchmarking try commit 28d741e, comparison URL.

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

Doesn't look like this is significant-enough perf regression, so we could probably even land it in compare mode.

| "saturating_add"
| "saturating_sub"
| "transmute"
=> return Err(Unpromotable),

This comment has been minimized.

Copy link
@RalfJung

RalfJung Aug 26, 2019

Member

Is this a blacklist of unpromotable intrinsics, or what is going on here?

Cc #61495

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 26, 2019

Author Member

It's copy-pasted from qualify_consts.

In general I can attempt to do more deduplication, and for that it would probably be best to make a consts module that handles qualification/checking/promotion, so that I can define a lot of pub(super) things.

But I wanted to have minimal reuse for the crater run to avoid "unaccounted for" logic.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Aug 26, 2019

Member

Sure. I was just surprised by the polarity here. I expected a whitelist, but this looks like a blacklist.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Aug 27, 2019

Member

In fact I think this list should just go. We don't want to promote any intrinsics.

The only intrinsic that stable code can directly access is transmute, and we certainly don't want to promote that. All of the others are exposed via wrapping functions, and we use #[rustc_promotable] to control their promotability.

@pietroalbini

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

@craterbot crates=full

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

📝 Configuration of the pr-63812 experiment changed.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

bors added a commit that referenced this pull request Sep 8, 2019
Auto merge of #63994 - Centril:refactor-qualify-consts, r=spastorino,…
…oli-obk

Refactor the `MirPass for QualifyAndPromoteConstants`

This is an accumulation of drive-by commits while working on `Vec::new` as a stable `const fn`.
The PR is probably easiest read commit-by-commit.

r? @oli-obk

cc @eddyb @ecstatic-morse -- your two PRs #63812 and #63860 respectively will conflict with this a tiny bit but it should be trivial to reintegrate your changes atop of this.

@eddyb eddyb force-pushed the eddyb:promo-sanity branch from 0229fb0 to 335859e Sep 12, 2019

bors added a commit that referenced this pull request Sep 12, 2019
Auto merge of #64398 - eddyb:crater-63809-63812-63831, r=<try>
[WIP] Crater rollup of 3 PRs (#63809, #63812, #63831).

I've combined my 3 PRs in the crater queue:
* #63809 proc_macro: check non-interned handles for "leaks" between/after invocations.
* #63812 rustc_mir: double-check const-promotion candidates for sanity.
* #63831 rustc_mir: disallow global mutable state in proc macros.

cc @pietroalbini @Mark-Simulacrum Is this a good idea?
@eddyb

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

@craterbot abort
(in favor of #64398)

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Sep 12, 2019

🗑 Experiment pr-63812 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.