refactor(mir-transform): Merge can_be_overridden, is_required and is_enabled into one

[clubby789]

View all comments

#128657 and #134082 both merged at similar times, and added two similar flags to the MirPass trait

• can_be_overridden: Controls if -Zmir-enable-passes can enable/disable a pass; only set to false for ForceInline
• is_required: Controls if #[optimize(none)] should suppress a pass; set to explicitly true/false across all passes based on which appeared to be unconditionally enabled at the time

This adds some redundancy, so this PR merges both into `can_be_overridden`: - removes cases of `is_required(&self) -> bool { true }` - replaces `is_required(&self) -> bool { false }` with `can_be_overridden(&self) -> bool { true }`

Also, previously, -Zmir-enable-passes took precedence, but now #[optimize(none)] does, as the latter is more local.

This merges both functions as well as is_enabled into a single PassPolicy enum

r? @RalfJung

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in coverage instrumentation.

cc @Zalathar

[rustbot]

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

[RalfJung]

Thanks for helping with the cleanup!

This adds some redundancy, so this PR merges both into can_be_overridden:

The two have slightly different semantics though, don't they?
For instance, CheckAlignment is a pass that can be turned off by -Zmir-enable-passes=-CheckAlignment, but that probably should not be turned off by #[optimize(none)].

So I think we need a 3-variant enum here: passes that are actually required (as they implement parts of the MIR semantics), passes that are optional but not "optimizations", and passes that are truly just optimizations. An actually required pass should have an is_enabled of true (which is the default but that's a bad default so we should change it, though that can be a future PR). Every pass marked as truly required should have a comment why it is required so this becomes easier to audit.

That still spreads the logic across both is_enabled and this 3-vairant classification so maybe this should be unified further into a single function to avoid the redundancy.

[RalfJung]

Every pass marked as truly required should have a comment why it is required so this becomes easier to audit.

FWIW I had started doing this until I realized that is_required does not actually mean "is required", it means "optional pass controlled by is_enabled that should be enabled even under #[optimize(none)]". But some of the comments I added could be useful. See b537849.

[RalfJung]

That still spreads the logic across both is_enabled and this 3-vairant classification so maybe this should be unified further into a single function to avoid the redundancy.

Thinking a bit more about this... one way we could represent this is with an enum like this:

enum PassEnabled {
  /// This pass implements a mandatory lowering step, either to implement parts of the MIR semantics
  /// or to bring MIR into a shape that is easier to deal with for later passes / codegen.
  /// Passes using this cannot be disable via any means. They must not remove any UB.
  Required,
  /// This pass is optional, it can be controlled via `-Zmir-enable-passes`.
  Optional {
    /// Whether the pass should be enabled in this compilation session by default,
    /// i.e., before applying `-Zmir-enable-passes` or `#[optimize(...)]`.
    default_enabled: bool,
    /// Whether this is an optimization pass. Only optimization passes are disabled by
    /// `#[optimize(none)]`. A pass can be optional without being an optimization pass,
    /// e.g. if it just adds extra debug checks that one can anyway also turn off.
    optimization: bool,
  }
}

One could then have a few convenient constructors for the typical cases to make this less verbose.

What I am missing in this proposal is a way to explicitly mark an optimization as "I promise not to remove UB". Maybe that should also be a field on PassEnabled::Optional?
Writing this up makes me wonder why we even distinguish between -Zmir-opt-level=0 and -Zmir-preserve-ub. Can't we just define opt-level 0 to be the "UB-preserving level"? There is anyway no way to get that level on stable, even debug builds default to mir-opt-level 1. -Zmir-preserve-ub currently does two things: it disables the RemovePlaceMention pass, and it changes the behavior of CfgSimplifier. It seems that makes CfgSimplifier a pass that may or may not remove UB depending on configuration, but we could deal with that by having the field be specified as follows:

/// If this is `true`, then the pass promises that *if `mir_opt_level() == 0`*, it will not remove any UB.
/// Passes that set this to `false` automatically get disabled at MIR optimization level 0.
preserves_ub: bool,

Cc @saethlin @davidtwco @fee1-dead

[clubby789]

I wrote up an approach revolving around the below enum, with is_enabled being removed and folded into enabled_by_default.
Folding Optimization/Optional into one may make sense, especially if we're adding a preserves_ub flag.

EDIT: Folded together

/// Rules outlining when this pass may be overridden or suppressed.
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub(crate) enum PassPolicy {
    /// Part of MIR semantics and must run, even in Miri.
    /// Such passes must therefore never remove UB, and should come with a comment justifying
    /// why they must always run.
    Required,
    /// Not an optimization, but may be configured by `-Zmir-enable-passes`.
    Optional {
        /// Whether this pass should be enabled by default in this session in the absence of
        /// an explicit `-Zmir-enable-passes`.
        enabled_by_default: bool,
    },
    /// An optimization pass, which may also be suppressed by `#[optimize(none)]`.
    Optimization {
        /// Whether this pass should be enabled by default in this session in the absence of
        /// an explicit `-Zmir-enable-passes` or `#[optimize]` attribute.
        enabled_by_default: bool,
    },
}

[RalfJung]

That should also work. Not sure how many "optional" passes we have and whether the UB removal question even makes sense for them.

[RalfJung]

This does not really help with ensuring that we don't run UB-removing passes in Miri... but that's fine, this PR does not have to solve all problems at once.

I haven't gone over every pass yet, here's a first round of comments.

View changes since this review

[RalfJung]

This now invokes the underlying passes policy which previously was not the case. Does that change behavior anywhere we use WithMinOptLevel?

[clubby789]

AFAICT, the one place this does interact is that RemoveNoopLandingPads had an is_enabled of panic_strategy().unwinds() and is_required of true. Previously, this condition was ignored by the wrapper. Now it respects it

[RalfJung]

CheckAlignment/CheckEnums/CheckNull (required=true, enabled=ub_checks() -> non_optimization(ub_checks()))

That's not a change?

[clubby789]

Indeed, I think I confused myself with the naming... I will see if it's possible to split the policy changes into a second commit

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[clubby789]

Split into two commits, the first should be only refactoring and the second introduces changes to the policies and adding justifications from your commit

[RalfJung]

Yes I like it. :) Just some comment nits.
@rustbot author

We probably want to further tweak optimize(none) behavior and maybe have a mir-opt level that disables all optional passes but that's further refactoring which does not have to happen in this PR.

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[RalfJung]

r=me when CI is green. :)
@bors delegate+

[rust-bors]

✌️ @clubby789, you can now approve this pull request!

If @RalfJung told you to "r=me" after making some further change, then please make that change and post @bors r=RalfJung.

View changes since this delegation.

[clubby789]

I will probably investigate #160057 (comment) as a followup, but I think this is a good start to clarify and correct the policies for now.

[clubby789]

@bors r=RalfJung

[rust-bors]

📌 Commit 6ec44cd has been approved by RalfJung

It is now in the queue for this repository.