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

dianqk · 2024-03-24T04:30:40Z

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

r? RalfJung

rustbot · 2024-03-24T04:30:48Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

RalfJung

Thanks!

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

RalfJung · 2024-03-24T08:15:28Z

library/core/src/intrinsics.rs

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


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.

RalfJung · 2024-03-24T08:16:32Z

library/core/src/intrinsics.rs

 /// 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`.


Suggested change

/// but with `rustc_preserve_ub_checks`.

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

RalfJung · 2024-03-24T08:18:35Z

compiler/rustc_mir_transform/src/instsimplify.rs


+    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);


Suggested change

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

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

RalfJung · 2024-03-24T08:25:26Z

library/core/src/intrinsics.rs

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


Suggested change

/// `cfg!(debug_assertions)` during monomorphization when using `rustc_preserve_ub_checks` or

/// `-Cdebug_assertions=yes`, otherwise it can evaluate at any time.

RalfJung · 2024-03-24T08:25:48Z

library/core/src/intrinsics.rs

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


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 · 2024-03-24T11:15:04Z

This PR changes MIR

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

dianqk · 2024-03-24T11:27:41Z

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

dianqk · 2024-03-24T11:32:34Z

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 · 2024-03-24T11:42:03Z

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.

tests/mir-opt/inline/unwrap_unchecked.unwrap_unchecked.PreCodegen.after.panic-abort.mir

RalfJung · 2024-03-24T13:25:06Z

compiler/rustc_middle/src/mir/syntax.rs

-    /// 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.


Suggested change

/// Refer to the comments in the `ub_checks` function.

/// See the `ub_checks` intrinsic docs for details.

dianqk · 2024-03-27T06:38:37Z

@bors try @rust-timer queue

bors · 2024-03-27T06:39:46Z

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

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 · 2024-03-27T08:17:50Z

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

rust-timer · 2024-03-27T11:45:45Z

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 · 2024-03-28T20:43:05Z

💔 Test failed - checks-actions

rust-log-analyzer · 2024-03-28T20:48:15Z

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 · 2024-03-28T21:14:48Z

During LLVM build, likely unrelated/spurious

Noratrieb · 2024-03-28T21:49:12Z

@bors retry #122671 (comment)

dianqk · 2024-03-29T01:14:44Z

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 · 2024-03-29T01:38:07Z

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 · 2024-03-29T02:25:46Z

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

bors · 2024-03-29T04:26:02Z

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

rust-timer · 2024-03-29T06:52:45Z

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 · 2024-03-29T11:14:15Z

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 · 2024-03-31T05:42:49Z

The pass ordering tweak experiment is: #123256

Run InstSimplify before UnreachablePropagation This is the pass ordering change I was musing about: rust-lang#122975 (comment) r? `@ghost`

dianqk · 2024-04-01T01:42:09Z

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 · 2024-04-01T08:02:19Z

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 · 2024-04-02T01:08:38Z

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.

rustbot assigned RalfJung Mar 24, 2024

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

dianqk mentioned this pull request Mar 24, 2024

Eliminate UbCheck for non-standard libraries #122282

Closed

dianqk force-pushed the simplify_ub_check branch from 02a2ef7 to 1f87613 Compare March 24, 2024 04:38

This comment has been minimized.

Sign in to view

RalfJung reviewed Mar 24, 2024

View reviewed changes

dianqk force-pushed the simplify_ub_check branch from 1f87613 to 39f2b11 Compare March 24, 2024 11:15

RalfJung reviewed Mar 24, 2024

View reviewed changes

tests/mir-opt/inline/unwrap_unchecked.unwrap_unchecked.PreCodegen.after.panic-abort.mir Show resolved Hide resolved

dianqk force-pushed the simplify_ub_check branch from 39f2b11 to 1f349ad Compare March 24, 2024 12:10

RalfJung reviewed Mar 24, 2024

View reviewed changes

dianqk force-pushed the simplify_ub_check branch from 1f349ad to 8823c37 Compare March 24, 2024 13:28

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

dianqk changed the title ~~Eliminate UbCheck for non-standard libraries~~ Eliminate UbChecks for non-standard libraries Mar 27, 2024

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

dianqk force-pushed the simplify_ub_check branch from 8823c37 to 7d7e9d4 Compare March 27, 2024 13:00

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

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

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 29, 2024

bors merged commit 760e567 into rust-lang:master Mar 29, 2024

rustbot added this to the 1.79.0 milestone Mar 29, 2024

dianqk deleted the simplify_ub_check branch March 29, 2024 04:27

This was referenced Mar 29, 2024

Shrink from_raw_parts's MIR so that Vec::deref MIR-inlines again #123190

Closed

Rework MIR inlining costs #123179

Draft

saethlin mentioned this pull request Mar 31, 2024

Run InstSimplify before UnreachablePropagation #123256

Closed

dtcxzyw mentioned this pull request Apr 11, 2024

[InstCombine] Simplify with.overflow intrinsics with assumption information llvm/llvm-project#84016

Merged

saethlin mentioned this pull request Apr 12, 2024

Totally-not-a-tracking-issue for UB-detecting debug assertions in the standard library #120848

Open

9 tasks

CAD97 mentioned this pull request Apr 16, 2024

Add an intrinsic for ptr::from_raw_parts(_mut) #123840

Merged

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

-/// 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.

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

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

		/// `cfg!(debug_assertions)` during monomorphization when using `rustc_preserve_ub_checks` or
		/// `-Cdebug_assertions=yes`, otherwise it can evaluate at 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

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

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

Uh oh!

Conversation

dianqk commented Mar 24, 2024

Uh oh!

rustbot commented Mar 24, 2024

Uh oh!

This comment has been minimized.

RalfJung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Mar 24, 2024

Uh oh!

dianqk commented Mar 24, 2024

Uh oh!

dianqk commented Mar 24, 2024

Uh oh!

RalfJung commented Mar 24, 2024

Uh oh!

Uh oh!

RalfJung Mar 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dianqk commented Mar 27, 2024

Uh oh!

This comment has been minimized.

bors commented Mar 27, 2024

Uh oh!

bors commented Mar 27, 2024

Uh oh!

This comment has been minimized.

rust-timer commented Mar 27, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Uh oh!

bors commented Mar 28, 2024

Uh oh!

rust-log-analyzer commented Mar 28, 2024

Uh oh!

CAD97 commented Mar 28, 2024

Uh oh!

Noratrieb commented Mar 28, 2024

Uh oh!

dianqk commented Mar 29, 2024

Uh oh!

dianqk commented Mar 29, 2024

Uh oh!

bors commented Mar 29, 2024

Uh oh!

bors commented Mar 29, 2024

Uh oh!

rust-timer commented Mar 29, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Uh oh!

RalfJung commented Mar 29, 2024

Uh oh!

saethlin commented Mar 31, 2024

Uh oh!

dianqk commented Apr 1, 2024

Uh oh!

RalfJung commented Apr 1, 2024

Uh oh!

dianqk commented Apr 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

RalfJung Mar 24, 2024 •

edited

Loading