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.

Sign up for GitHub

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

Jump to bottom

Don't run various MIR optimizations at mir-opt-level=0 #70073

Merged
merged 1 commit into from
Apr 27, 2020
Merged

Don't run various MIR optimizations at mir-opt-level=0 #70073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/librustc_mir/shim.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,13 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> &'tcx
instance,
None,
MirPhase::Const,
&[
&[&[
&add_moves_for_packed_drops::AddMovesForPackedDrops,
&no_landing_pads::NoLandingPads::new(tcx),
&remove_noop_landing_pads::RemoveNoopLandingPads,
&simplify::SimplifyCfg::new("make_shim"),
&add_call_guards::CriticalCallEdges,
],
]],
);

debug!("make_shim({:?}) = {:?}", instance, result);
Expand Down
5 changes: 0 additions & 5 deletions src/librustc_mir/transform/instcombine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,6 @@ pub struct InstCombine;

impl<'tcx> MirPass<'tcx> for InstCombine {
fn run_pass(&self, tcx: TyCtxt<'tcx>, _: MirSource<'tcx>, body: &mut Body<'tcx>) {
// We only run when optimizing MIR (at any level).
if tcx.sess.opts.debugging_opts.mir_opt_level == 0 {
return;
}

// First, find optimization opportunities. This is done in a pre-pass to keep the MIR
// read-only so that we can do global analyses on the MIR in the process (e.g.
// `Place::ty()`).
Expand Down
127 changes: 79 additions & 48 deletions src/librustc_mir/transform/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ pub fn run_passes(
instance: InstanceDef<'tcx>,
promoted: Option<Promoted>,
mir_phase: MirPhase,
passes: &[&dyn MirPass<'tcx>],
passes: &[&[&dyn MirPass<'tcx>]],
) {
let phase_index = mir_phase.phase_index();

Expand Down Expand Up @@ -168,8 +168,10 @@ pub fn run_passes(
index += 1;
};

for pass in passes {
run_pass(*pass);
for pass_group in passes {
for pass in *pass_group {
run_pass(*pass);
}
}

body.phase = mir_phase;
Expand Down Expand Up @@ -219,11 +221,11 @@ fn mir_const(tcx: TyCtxt<'_>, def_id: DefId) -> &Steal<Body<'_>> {
InstanceDef::Item(def_id),
None,
MirPhase::Const,
&[
&[&[
// What we need to do constant evaluation.
&simplify::SimplifyCfg::new("initial"),
&rustc_peek::SanityCheck,
],
]],
);
tcx.alloc_steal_mir(body)
}
Expand All @@ -244,11 +246,11 @@ fn mir_validated(
InstanceDef::Item(def_id),
None,
MirPhase::Validated,
&[
&[&[
// What we need to run borrowck etc.
&promote_pass,
&simplify::SimplifyCfg::new("qualify-consts"),
],
]],
);

let promoted = promote_pass.promoted_fragments.into_inner();
Expand All @@ -261,54 +263,83 @@ fn run_optimization_passes<'tcx>(
def_id: DefId,
promoted: Option<Promoted>,
) {
let post_borrowck_cleanup: &[&dyn MirPass<'tcx>] = &[
// Remove all things only needed by analysis
&no_landing_pads::NoLandingPads::new(tcx),
&simplify_branches::SimplifyBranches::new("initial"),
&remove_noop_landing_pads::RemoveNoopLandingPads,
&cleanup_post_borrowck::CleanupNonCodegenStatements,
&simplify::SimplifyCfg::new("early-opt"),
// These next passes must be executed together
&add_call_guards::CriticalCallEdges,
&elaborate_drops::ElaborateDrops,
&no_landing_pads::NoLandingPads::new(tcx),
// AddMovesForPackedDrops needs to run after drop
// elaboration.
&add_moves_for_packed_drops::AddMovesForPackedDrops,
// `AddRetag` needs to run after `ElaborateDrops`. Otherwise it should run fairly late,
// but before optimizations begin.
&add_retag::AddRetag,
&simplify::SimplifyCfg::new("elaborate-drops"),
// No lifetime analysis based on borrowing can be done from here on out.
];

let optimizations: &[&dyn MirPass<'tcx>] = &[
&unreachable_prop::UnreachablePropagation,
&uninhabited_enum_branching::UninhabitedEnumBranching,
&simplify::SimplifyCfg::new("after-uninhabited-enum-branching"),
&inline::Inline,
// Lowering generator control-flow and variables has to happen before we do anything else
// to them. We do this inside the "optimizations" block so that it can benefit from
// optimizations that run before, that might be harder to do on the state machine than MIR
// with async primitives.
&generator::StateTransform,
&instcombine::InstCombine,
&const_prop::ConstProp,
Copy link
Member

Choose a reason for hiding this comment

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

ConstProp contains a test for mir_opt_level, should that also be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm.... I guess we should either remove the check since we don't run it if optimizations are disabled or we should add it to no_optimizations leaving the check. I think the second option would be the same as the current behavior, so I'm going to do that.

Copy link
Member

Choose a reason for hiding this comment

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

Okay. Please leave a FIXME though, to evaluate if this actually doing anything with optimizations turned off. (That pass also does some linting, but then CI is passing in your PR so it seems we already don't get those lints... or we just never test mir-opt-level=0?)

Copy link
Member Author

Choose a reason for hiding this comment

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

We never test mir-opt-level=0 and mir-opt-level=2 is only tested by the mir-opt tests.

Copy link
Member

Choose a reason for hiding this comment

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

We never test mir-opt-level=0

That is rather concerning, in particular considering that StateTransform might rely on some of the simplifications that are now being skipped having happened (and #70073 (comment)).

(Miri sets mir-opt-level=0 though, so those code paths are not entirely untested.)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would make sense to take some of the basic async fn smoketests in the rustc test suite, and use the revision system to also run them with mir-opt-level=0?

Copy link
Member

Choose a reason for hiding this comment

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

I think that should look something like

// revisions: default nomiropt
//[nomiropt]compile-flags: -Z mir-opt-level=0

I usually add -Zdoesntexist as well for a first test just to make sure the flag is actually used.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's definitely not ideal. A compiler bootstrap with mir-opt-level=0 does complete successfully though so it's not completely broken.

&simplify_branches::SimplifyBranches::new("after-const-prop"),
// Run deaggregation here because:
// 1. Some codegen backends require it
// 2. It creates additional possibilities for some MIR optimizations to trigger
wesleywiser marked this conversation as resolved.
Show resolved Hide resolved
// FIXME(#70073): Why is this done here and not in `post_borrowck_cleanup`?
&deaggregator::Deaggregator,
wesleywiser marked this conversation as resolved.
Show resolved Hide resolved
&copy_prop::CopyPropagation,
&simplify_branches::SimplifyBranches::new("after-copy-prop"),
&remove_noop_landing_pads::RemoveNoopLandingPads,
&simplify::SimplifyCfg::new("after-remove-noop-landing-pads"),
&simplify_try::SimplifyArmIdentity,
&simplify_try::SimplifyBranchSame,
&simplify::SimplifyCfg::new("final"),
&simplify::SimplifyLocals,
];

let no_optimizations: &[&dyn MirPass<'tcx>] = &[
// Even if we don't do optimizations, we still have to lower generators for codegen.
&generator::StateTransform,
// FIXME(#70073): This pass is responsible for both optimization as well as some lints.
&const_prop::ConstProp,
// Even if we don't do optimizations, still run deaggregation because some backends assume
// that deaggregation always occurs.
&deaggregator::Deaggregator,
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
];

let pre_codegen_cleanup: &[&dyn MirPass<'tcx>] = &[
&add_call_guards::CriticalCallEdges,
// Dump the end result for testing and debugging purposes.
&dump_mir::Marker("PreCodegen"),
];

let mir_opt_level = tcx.sess.opts.debugging_opts.mir_opt_level;

run_passes(
tcx,
body,
InstanceDef::Item(def_id),
promoted,
MirPhase::Optimized,
&[
// Remove all things only needed by analysis
&no_landing_pads::NoLandingPads::new(tcx),
&simplify_branches::SimplifyBranches::new("initial"),
&remove_noop_landing_pads::RemoveNoopLandingPads,
&cleanup_post_borrowck::CleanupNonCodegenStatements,
&simplify::SimplifyCfg::new("early-opt"),
// These next passes must be executed together
&add_call_guards::CriticalCallEdges,
&elaborate_drops::ElaborateDrops,
&no_landing_pads::NoLandingPads::new(tcx),
// AddMovesForPackedDrops needs to run after drop
// elaboration.
&add_moves_for_packed_drops::AddMovesForPackedDrops,
// `AddRetag` needs to run after `ElaborateDrops`. Otherwise it should run fairly late,
// but before optimizations begin.
&add_retag::AddRetag,
&simplify::SimplifyCfg::new("elaborate-drops"),
// No lifetime analysis based on borrowing can be done from here on out.

// Optimizations begin.
&unreachable_prop::UnreachablePropagation,
&uninhabited_enum_branching::UninhabitedEnumBranching,
&simplify::SimplifyCfg::new("after-uninhabited-enum-branching"),
&inline::Inline,
// Lowering generator control-flow and variables
// has to happen before we do anything else to them.
&generator::StateTransform,
&instcombine::InstCombine,
&const_prop::ConstProp,
&simplify_branches::SimplifyBranches::new("after-const-prop"),
&deaggregator::Deaggregator,
&copy_prop::CopyPropagation,
&simplify_branches::SimplifyBranches::new("after-copy-prop"),
&remove_noop_landing_pads::RemoveNoopLandingPads,
&simplify::SimplifyCfg::new("after-remove-noop-landing-pads"),
&simplify_try::SimplifyArmIdentity,
&simplify_try::SimplifyBranchSame,
&simplify::SimplifyCfg::new("final"),
&simplify::SimplifyLocals,
&add_call_guards::CriticalCallEdges,
&dump_mir::Marker("PreCodegen"),
post_borrowck_cleanup,
if mir_opt_level > 0 { optimizations } else { no_optimizations },
pre_codegen_cleanup,
],
);
}
Expand Down