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

Read from non-scalar constants and statics in dataflow const-prop #115705

Merged
merged 10 commits into from
Sep 12, 2023

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Sep 9, 2023

DataflowConstProp is designed to handle scalar values. When MIR features an assignment from a non-scalar constant, we need to manually decompose it into the custom state space.

This PR tweaks interpreter callbacks to allow reusing eval_mir_constant without having a stack frame to get a span from.

r? @oli-obk
cc @jachris

@rustbot rustbot added 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 Sep 9, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 9, 2023

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer

This comment has been minimized.

impl<'mir, 'tcx: 'mir> rustc_const_eval::interpret::Machine<'mir, 'tcx> for DummyMachine {
rustc_const_eval::interpret::compile_time_machine!(<'mir, 'tcx>);
type MemoryKind = !;
const PANIC_ON_ALLOC_FAIL: bool = true;

#[inline(always)]
fn cur_span(_ecx: &InterpCx<'mir, 'tcx, Self>) -> Span {
Copy link
Member

@RalfJung RalfJung Sep 9, 2023

Choose a reason for hiding this comment

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

Hm, I wonder if this could also help avoid this nonsense in the old const-prop-lint pass

// Overwrite the PC -- whatever the interpreter does to it does not make any sense anyway.
self.ecx.frame_mut().loc = Left(location);

Specifically, we really should make loc private, maybe we can with this hook.

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2023

This PR tweaks interpreter callbacks to allow reusing eval_mir_constant without having a stack frame to get a span from.

eval_mir_constant already has an Option<Span> parameter, is that not useful?

@cjgillot
Copy link
Contributor Author

cjgillot commented Sep 9, 2023

eval_mir_constant already has an Option<Span> parameter, is that not useful?

The Option<Span> states whether and where to emit the "erroneous constant used" diagnostic. We pass None as we do not want a diagnostic.
cur_span is used for query cycles. Using an empty stack seems to work though.

@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot added the A-mir-opt Area: MIR optimizations label Sep 10, 2023
Copy link
Contributor

@oli-obk oli-obk 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

compiler/rustc_const_eval/src/interpret/intrinsics.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor

oli-obk commented Sep 11, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 11, 2023

📌 Commit 6984030 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 Sep 11, 2023
@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 Sep 11, 2023
@bors
Copy link
Contributor

bors commented Sep 12, 2023

⌛ Testing commit 6984030 with merge cc7a9d6...

@bors
Copy link
Contributor

bors commented Sep 12, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 12, 2023
@bors bors merged commit cc7a9d6 into rust-lang:master Sep 12, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 12, 2023
@cjgillot cjgillot deleted the const-prop-aggregate branch September 12, 2023 10:53
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cc7a9d6): 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)
3.6% [3.0%, 4.4%] 3
Improvements ✅
(primary)
-3.4% [-3.4%, -3.4%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.4% [-3.4%, -3.4%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.075s -> 632.792s (0.27%)
Artifact size: 317.84 MiB -> 317.95 MiB (0.04%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt Area: MIR optimizations 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-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.

7 participants