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

Eliminate UbChecks for non-standard libraries #122975

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

DianQK
Copy link
Member

@DianQK DianQK commented Mar 24, 2024

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

r? RalfJung

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 24, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@RalfJung RalfJung left a comment

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<T>(x: *mut T, y: *mut T) {
}

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

Choose a reason for hiding this comment

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

Suggested change
/// 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`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// 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);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// `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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// 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

@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2024

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

@DianQK
Copy link
Member Author

DianQK commented Mar 24, 2024

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

@DianQK
Copy link
Member Author

DianQK commented Mar 24, 2024

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.

@RalfJung
Copy link
Member

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.
Copy link
Member

@RalfJung RalfJung Mar 24, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! :3

@DianQK
Copy link
Member Author

DianQK commented Mar 27, 2024

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 27, 2024
@bors
Copy link
Contributor

bors commented Mar 27, 2024

⌛ Trying commit 8823c37 with merge 98cd703...

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
@bors
Copy link
Contributor

bors commented Mar 27, 2024

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

@rust-timer

This comment has been minimized.

@DianQK DianQK changed the title Eliminate UbCheck for non-standard libraries Eliminate UbChecks for non-standard libraries Mar 27, 2024
@rust-timer
Copy link
Collaborator

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%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 27, 2024
@bors
Copy link
Contributor

bors commented Mar 28, 2024

💔 Test failed - checks-actions

@bors 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
@rust-log-analyzer
Copy link
Collaborator

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

@CAD97
Copy link
Contributor

CAD97 commented Mar 28, 2024

During LLVM build, likely unrelated/spurious

@Noratrieb
Copy link
Member

@bors retry #122671 (comment)

@bors 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
@DianQK
Copy link
Member Author

DianQK commented Mar 29, 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.

@DianQK
Copy link
Member Author

DianQK commented Mar 29, 2024

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.

@bors
Copy link
Contributor

bors commented Mar 29, 2024

⌛ Testing commit 47ed73a with merge 760e567...

@bors
Copy link
Contributor

bors commented Mar 29, 2024

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing 760e567 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 29, 2024
@bors bors merged commit 760e567 into rust-lang:master Mar 29, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 29, 2024
@DianQK DianQK deleted the simplify_ub_check branch March 29, 2024 04:27
@rust-timer
Copy link
Collaborator

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%)

@RalfJung
Copy link
Member

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.

@saethlin
Copy link
Member

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`
@DianQK
Copy link
Member Author

DianQK commented Apr 1, 2024

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.

@RalfJung
Copy link
Member

RalfJung commented Apr 1, 2024

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.

@DianQK
Copy link
Member Author

DianQK commented Apr 2, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants