Eliminate `UbChecks` for non-standard libraries by dianqk · Pull Request #122975 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation49 Commits1 Checks11 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

The purpose of this PR is to allow other passes to treat UbChecks as constants in MIR for optimization after #122629.

r? RalfJung

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.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

Mar 24, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This comment has been minimized.

Choose a reason for hiding this comment

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

Thanks!

There's also a comment at the enum NullOp that needs updating.

@@ -2662,11 +2662,13 @@ pub const unsafe fn typed_swap(x: mut T, y: mut T) {
}

/// Returns whether we should perform some UB-checking at runtime. This evaluate to the value of

Choose a reason for hiding this comment

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

/// Returns whether we should perform some UB-checking at runtime. This evaluate to the value of
/// Returns whether we should perform some UB-checking at runtime. This eventually evaluates to
/// `cfg!(debug_assertions)`, but behaves different from `cfg!` when mixing crates built with different
/// flags: if the crate has debug assertions enabled or carries the `#[rustc_preserve_ub_checks]`
/// attribute, evaluation is delayed until monomorphization (or until the call gets inlined into
/// a crate that does not delay evaluation further); otherwise it can happen any time.

///
/// This intrinsic is evaluated after monomorphization, which is relevant when mixing crates
/// compiled with and without debug_assertions. The common case here is a user program built with
/// debug_assertions linked against the distributed sysroot which is built without debug_assertions.
/// debug_assertions linked against the distributed sysroot which is built without debug_assertions
/// but with `rustc_preserve_ub_checks`.

Choose a reason for hiding this comment

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

/// but with `rustc_preserve_ub_checks`.
/// but with `#[rustc_preserve_ub_checks]`.

@@ -140,6 +149,14 @@ impl<'tcx> InstSimplifyContext<'tcx, '_> {
}
}

fn simplify_ub_check(&self, source_info: &SourceInfo, rvalue: &mut Rvalue<'tcx>) {
if let Rvalue::NullaryOp(NullOp::UbChecks, _) = *rvalue {
let const_ = Const::from_bool(self.tcx, false);

Choose a reason for hiding this comment

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

let const_ = Const::from_bool(self.tcx, false);
let const_ = Const::from_bool(self.tcx, tcx.sess.opts.debug_assertions);

Comment on lines 2665 to 2666

/// `cfg!(debug_assertions)` during monomorphization when using `rustc_preserve_ub_checks` or
/// `-Cdebug_assertions=yes`, otherwise it can evaluate at any time.

Choose a reason for hiding this comment

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

/// `cfg!(debug_assertions)` during monomorphization when using `rustc_preserve_ub_checks` or
/// `-Cdebug_assertions=yes`, otherwise it can evaluate at any time.

Comment on lines 2668 to 2669

/// This intrinsic is evaluated after monomorphization, which is relevant when mixing crates
/// compiled with and without debug_assertions. The common case here is a user program built with

Choose a reason for hiding this comment

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

/// This intrinsic is evaluated after monomorphization, which is relevant when mixing crates
/// compiled with and without debug_assertions. The common case here is a user program built with
/// The common case here is a user program built with

Thanks for the detailed comments. I've fixed the failed test case. I should also add #[rustc_preserve_ub_checks] to alloc.

Thanks!

There's also a comment at the enum NullOp that needs updating.

I'm not sure how to keep the comment synchronized; I copied a section and added a citation.

I'm not sure how to keep the comment synchronized; I copied a section and added a citation.

Maybe it should just reference the intrinsic and not repeat anything.

/// Returns whether we want to check for UB.
/// This returns the value of `cfg!(debug_assertions)` at monomorphization time.
/// Returns whether we should perform some UB-checking at runtime.
/// Refer to the comments in the `ub_checks` function.

Choose a reason for hiding this comment

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

/// Refer to the comments in the `ub_checks` function.
/// See the `ub_checks` intrinsic docs for details.

Choose a reason for hiding this comment

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

Thanks! :3

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 27, 2024

Eliminate UbCheck for non-standard libraries

The purpose of this PR is to allow other passes to treat UbChecks as constants in MIR for optimization after rust-lang#122629.

r? RalfJung

☀️ Try build successful - checks-actions
Build commit: 98cd703 (98cd70349125c1089476ac61546ec119beea56cf)

This comment has been minimized.

dianqk changed the title~~Eliminate UbCheck for non-standard libraries~~ Eliminate UbChecks for non-standard libraries

Mar 27, 2024

Finished benchmarking commit (98cd703): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean	range	count
Regressions ❌ (primary)	0.6%	[0.4%, 1.0%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.6%, -0.5%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.6%, 1.0%]	6

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)	2.7%	[2.1%, 3.3%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.9%	[-5.0%, -0.1%]	15
Improvements ✅ (secondary)	-3.1%	[-5.2%, -2.1%]	23
All ❌✅ (primary)	-1.3%	[-5.0%, 3.3%]	17

Cycles

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.8%	[0.7%, 0.9%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.9%	[-1.1%, -0.8%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-1.1%, 0.9%]	5

Binary size

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.2%	[0.0%, 0.7%]	39
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.0%]	13
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.1%, 0.7%]	52

Bootstrap: 670.407s -> 670.602s (0.03%)
Artifact size: 315.71 MiB -> 315.65 MiB (-0.02%)

bors added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

labels

Mar 28, 2024

The job dist-aarch64-msvc failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

-- Configuring done (39.7s)
-- Generating done (3.0s)
-- Build files have been written to: C:/a/rust/rust/build/x86_64-pc-windows-msvc/llvm/build
running: "cmake" "--build" "." "--target" "install" "--config" "Release" "--" "-j" "8"
ninja: error: build.ninja:3780: multiple outputs aren't (yet?) supported by depslog; bring this up on the mailing list if it affects you
thread 'main' panicked at C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\cmake-0.1.48\src\lib.rs:975:5:

command did not execute successfully, got: exit code: 1

During LLVM build, likely unrelated/spurious

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

Mar 28, 2024

I think there's interesting follow-up work to do here with tweaking the order this transform is applied, I think it might be better before UnreachablePropagation, but that change could result in messy perf reports so I'm happy landing this as-is.

Will you do this on #122610 or a new PR? I believe it's also acceptable to run such passes repeatedly.

Drive-by note: if we're drawing analogy to overflow checks, instead of a crate-wide #![rustc_preserve_ub_checks], this might want for a per-function #[rustc_inherit_ub_checks] configuration (cf. #[rustc_inherit_overflow_checks]). That would permit e.g. Vec::deref to cover the precondition check done by slice::from_raw_parts with its own invariants, which are presumably assert_unsafe_precondition! tested already when Vec::from_raw_parts is called, making the check in Vec::deref (which is blocking inlining, at least without this change) redundant.

Something to try on top of this change once it merges, I suppose. (I also could've easily misunderstood what this patch is doing.)

I think the #![rustc_preserve_ub_checks] attribute is similar to -Coverflow-checks=true/false. const_ub_checks seems to do something similar.

dianqk deleted the simplify_ub_check branch

March 29, 2024 04:27

Finished benchmarking commit (760e567): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean	range	count
Regressions ❌ (primary)	0.9%	[0.3%, 1.7%]	7
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.8%, -0.4%]	5
Improvements ✅ (secondary)	-0.2%	[-0.3%, -0.1%]	2
All ❌✅ (primary)	0.3%	[-0.8%, 1.7%]	12

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)	2.9%	[2.9%, 2.9%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.5%	[-2.2%, -0.9%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-2.2%, 2.9%]	3

Cycles

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)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-3.1%	[-3.1%, -3.1%]	1
All ❌✅ (primary)	0.1%	[-1.0%, 1.1%]	2

Binary size

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.2%	[0.0%, 1.1%]	39
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-1.2%, -0.0%]	11
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-1.2%, 1.1%]	50

Bootstrap: 669.604s -> 667.865s (-0.26%)
Artifact size: 315.78 MiB -> 315.66 MiB (-0.04%)

I think the #![rustc_preserve_ub_checks] attribute is similar to -Coverflow-checks=true/false. const_ub_checks seems to do something similar.

He wasn't talking about -Coverflow-checks, he was talking about #[rustc_inherit_overflow_checks]. I agree it may make sense to explore something like that for UB checks as well.

This was referenced

Mar 29, 2024

The pass ordering tweak experiment is: #123256

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 31, 2024

Run InstSimplify before UnreachablePropagation

This is the pass ordering change I was musing about: rust-lang#122975 (comment)

r? @ghost

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 31, 2024

Run InstSimplify before UnreachablePropagation

This is the pass ordering change I was musing about: rust-lang#122975 (comment)

r? @ghost

I think the #![rustc_preserve_ub_checks] attribute is similar to -Coverflow-checks=true/false. const_ub_checks seems to do something similar.

He wasn't talking about -Coverflow-checks, he was talking about #[rustc_inherit_overflow_checks]. I agree it may make sense to explore something like that for UB checks as well.

I think I understand now. The #[rustc_inherit_ub_checks] attribute is used to retain UBChecks even without the #![rustc_preserve_ub_checks] attribute. I think we can add it once we find a use case.

I think the more interesting variant would be to say that

crate-level #![rustc_preserve_ub_checks] basically means all functions get "inherit UB checks" treatment
but some functions can opt-out via #[rustc_dont_preserve_ub_checks], so e.g. Vec::deref can be optimized more.

Note that I have not checked whether this would even help for Vec::deref. It was just brought up in the discussion. This definitely needs a bit more design and exploration before someone attempts an implementation.

I think the more interesting variant would be to say that
* crate-level `#![rustc_preserve_ub_checks]` basically means all functions get "inherit UB checks" treatment

* _but_ some functions can opt-out via `#[rustc_dont_preserve_ub_checks]`, so e.g. `Vec::deref` can be optimized more.
Note that I have not checked whether this would even help for Vec::deref. It was just brought up in the discussion. This definitely needs a bit more design and exploration before someone attempts an implementation.

Great. This seems helpful for the internals of the standard library.

CAD97 mentioned this pull request