Eliminate `UbCheck` for non-standard libraries #122282

DianQK · 2024-03-10T09:55:48Z

I think it's only necessary to keep UB checks for core and std because only they are prebuilt crates,
mir-opt may not optimize some code due to UbCheck.
The purpose of this PR is to allow other passes to treat UbCheck as constants in MIR for optimization.
In more complex cases, I'm also not sure that LLVM utilizes assume correctly.

r? saethlin

rustbot · 2024-03-10T09:55:57Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler/rustc_feature/src/builtin_attrs.rs

compiler/rustc_feature/src/unstable.rs

DianQK · 2024-03-10T14:26:52Z

@bors try @rust-timer queue

bors · 2024-03-10T14:28:01Z

⌛ Trying commit e6474b8 with merge acc30d0...

Eliminate `UbCheck` for non-standard libraries Since only core and std are prebuilt crates, I think it's only necessary to keep UB checks for them. `mir-opt` may not optimize some code due to `UbCheck`. In more complex cases, I'm also not sure that LLVM utilizes `assume` correctly. r? saethlin

bors · 2024-03-10T15:55:29Z

☀️ Try build successful - checks-actions
Build commit: acc30d0 (acc30d0cafd012430f7d0d83addb6fea4c7b2bbf)

compiler/rustc_mir_transform/src/instsimplify.rs

saethlin · 2024-03-10T16:23:00Z

In more complex cases, I'm also not sure that LLVM utilizes assume correctly.

I'd really like to have an example of this, if you think it's happening. I think this other PR of mine should ensure that LLVM never sees the checks when they are not enabled: #121421. But if the problem is that LLVM using assume well relies on sufficient MIR inlining happening, that's bad (most of the MIR opt regressions from these checks are because of the inlining cost of the new calls).

I also tried a different kind of approach to eliminate the impact of the checks on MIR optimizations: #121767 I expected to see perf improvements as a result of getting the optimized MIR back to where it was before, but that didn't happen. Hopefully this approach works better 🤞

rust-timer · 2024-03-10T22:33:26Z

Finished benchmarking commit (acc30d0): 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.8%	[0.2%, 2.2%]	7
Regressions ❌ (secondary)	0.5%	[0.3%, 0.9%]	8
Improvements ✅ (primary)	-0.7%	[-2.0%, -0.2%]	16
Improvements ✅ (secondary)	-1.8%	[-4.3%, -0.5%]	10
All ❌✅ (primary)	-0.3%	[-2.0%, 2.2%]	23

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)	8.4%	[4.1%, 15.3%]	6
Regressions ❌ (secondary)	2.7%	[1.4%, 4.5%]	5
Improvements ✅ (primary)	-3.3%	[-9.6%, -0.2%]	13
Improvements ✅ (secondary)	-2.9%	[-5.3%, -1.4%]	28
All ❌✅ (primary)	0.4%	[-9.6%, 15.3%]	19

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.7%	[1.5%, 1.9%]	2
Regressions ❌ (secondary)	1.1%	[1.1%, 1.1%]	1
Improvements ✅ (primary)	-1.6%	[-2.0%, -1.2%]	4
Improvements ✅ (secondary)	-3.1%	[-4.1%, -2.6%]	3
All ❌✅ (primary)	-0.5%	[-2.0%, 1.9%]	6

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.6%	[0.0%, 5.0%]	63
Regressions ❌ (secondary)	0.5%	[0.4%, 0.8%]	31
Improvements ✅ (primary)	-0.5%	[-1.8%, -0.0%]	33
Improvements ✅ (secondary)	-1.9%	[-3.4%, -0.3%]	16
All ❌✅ (primary)	0.3%	[-1.8%, 5.0%]	96

Bootstrap: 648.121s -> 643.906s (-0.65%)
Artifact size: 309.99 MiB -> 310.08 MiB (0.03%)

DianQK · 2024-03-11T01:44:14Z

I'd really like to have an example of this, if you think it's happening. I think this other PR of mine should ensure that LLVM never sees the checks when they are not enabled: #121421. But if the problem is that LLVM using assume well relies on sufficient MIR inlining happening, that's bad (most of the MIR opt regressions from these checks are because of the inlining cost of the new calls).

The purpose of this PR come from rust-lang/hashbrown#509. Current LLVM does not handle assume well. (Of course there is a PR that is improving it.)

Based on my discussion with @dtcxzyw offline, it would be easier for LLVM to handle instructions than intrinsic functions. We may miss out some optimization. If I can remove UbCheck in mir, I should be able to transform the following code to an add instruction.

x.checked_add(y).unwrap_unchecked()

compiler/rustc_feature/src/builtin_attrs.rs

compiler/rustc_mir_transform/src/instsimplify.rs

saethlin · 2024-03-11T03:17:10Z

Are you saying you're trying to write a MIR optimization or something like one so that rustc emits the LLVM IR that's easier for LLVM to optimize well, and UbCheck is impeding your MIR optimization?

So long as I understand that correctly I'm in favor of this, I just have the comments I left above (and much farther above). Usually attributes are more of a verb or a whole sentence, and rustc_ub_check is more like a noun or subject (It seems to ask: do what with rustc UB checks?)

DianQK · 2024-03-11T13:54:49Z

Are you saying you're trying to write a MIR optimization or something like one so that rustc emits the LLVM IR that's easier for LLVM to optimize well, and UbCheck is impeding your MIR optimization?

Yes. See https://rust.godbolt.org/z/YG6YrosEb. We can know bb2 is unreachable, then _4 and _6 is zero. Eventually, we can change CheckedAdd to Add. (Of course, this transformation can also be done in LLVM.) Maybe we can also fix #107681.

So long as I understand that correctly I'm in favor of this, I just have the comments I left above (and much farther above). Usually attributes are more of a verb or a whole sentence, and rustc_ub_check is more like a noun or subject (It seems to ask: do what with rustc UB checks?)

Thanks! I get it.

saethlin · 2024-03-11T21:32:19Z

Very nice.

@bors r+

saethlin · 2024-03-15T00:17:21Z

This is a MIR optimization, and we already deliberately use -Zmir-opt-level=0 in Miri to turn off MIR optimizations that may hide UB. This optimizations is just another aspect of that previous situation, so yes I agree this is a problem, but the solution is in changing the docs not the implementation. Checks may be optimized out, if you want to be sure they are all on, make sure you have all optimizations off.

RalfJung · 2024-03-15T06:49:17Z

This will add checks in Miri when mir-opts are enabled, not remove them. I think it makes the situation wrt when the checks are actually run way too messy (there's like 3 or 4 places that together define the behavior of this and one has to know all of them to understand what happens, and even you seem to have gotten wrong how they will interact). That is why I suggested above how this could be done with less cross-compiler-entanglement.

Or put differently, this is a mir-opt that fundamentally alters the semantics of these intrinsics, and I don't think their new semantics are a good choice.

DianQK · 2024-03-15T10:00:04Z

No. I think the main issue with this is that we distribute the prebuilt std and core library. I can evaluate Ubcheck in MIR if it from user-added crates that are built from source.

Now you are making the assumption that users will never mix rmeta files built with different values for -C debug-assertions. That's the sense in which you are making this best-effort.

I would expect that A does not get a UbCheck from B if A(-C debug-assertions=yes) depends on B(-C debug-assertions=no). Or what other scenes are being destroyed here?

RalfJung · 2024-03-15T11:13:19Z

I would expect that A does not get a UbCheck from B if A(-C debug-assertions=yes) depends on B(-C debug-assertions=no). Or what other scenes are being destroyed here?

The entire point of these intrinsics is to still make A get UB checks in that case! Here, B is the standard library.

If B doesn't want UbCheck in such cases it should just use cfg!(debug_assertions) rather than intrinsics::debug_assertions().

DianQK · 2024-03-15T13:50:34Z

I would expect that A does not get a UbCheck from B if A(-C debug-assertions=yes) depends on B(-C debug-assertions=no). Or what other scenes are being destroyed here?

The entire point of these intrinsics is to still make A get UB checks in that case! Here, B is the standard library.

IIUC, this is the current behavior of the master branch. This PR still retains this behavior. This PR will only work on BB inlined from std.

If B doesn't want UbCheck in such cases it should just use cfg!(debug_assertions) rather than intrinsics::debug_assertions().

Maybe I got your point. :3 We should consider replacing intrinsics::debug_assertions() to intrinsics::ub_check(). That seems to be what you are planning on up there as well?

RalfJung · 2024-03-15T14:47:45Z

IIUC, this is the current behavior of the master branch. This PR still retains this behavior. This PR will only work on BB inlined from std.

No, the PR does not retain this behavior. MIR-opts will now use the flags of the crate containing the MIR to resolve the intrinsic (except in the standard library), rather than the crate doing the codegen. I'm confused why you would say this PR does not change behavior when the entire point of this PR is to change behavior.

Currently, the behavior is as intended for all crates B. After this PR, it only works when B is the standard library. That's changing the documented contract of the intrinsic. The intrinsic is unstable so we can change the contract, but I'm saying the new contract is too fragile and non-local and I proposed an alternative that is IMO better.

We should consider replacing intrinsics::debug_assertions() to intrinsics::ub_check(). That seems to be what you are planning on up there as well?

That's just a rename, so it changes nothing about our discussion.

The first step is to agree on what the intended behavior of the intrinsics should be. I like what they currently are. I understand they should be changed to make mir-opt more effective. I'm okay with changing them but not with the change proposed here, primarily because it can lead to Miri running checks that we document and intend for Miri to never run. The current behavior for check_language_ub is:

Always return false in Miri; otherwise return the value of debug_assertions at codegen time.

The new behavior is:

Sometimes return the value of debug_assertions at mir-opt time (but only when optimizing outside the standard library, i.e. either the standard library code got inlined into outside code or someone used the unstable intrinsic from outside the standard library); if it happens to not get optimized, return false in Miri; otherwise return the value of debug_assertions at codegen time.

That's just not a sensible intrinsic IMO. It can be improved by changing the way Miri enters the picture: making Miri not part of how the intrinsic behaves but part of how it is used. As a side-benefit we also don't need two of these intrinsics any more.

DianQK · 2024-03-16T02:10:01Z

IIUC, this is the current behavior of the master branch. This PR still retains this behavior. This PR will only work on BB inlined from std.

No, the PR does not retain this behavior. MIR-opts will now use the flags of the crate containing the MIR to resolve the intrinsic (except in the standard library), rather than the crate doing the codegen. I'm confused why you would say this PR does not change behavior when the entire point of this PR is to change behavior.

Currently, the behavior is as intended for all crates B. After this PR, it only works when B is the standard library. That's changing the documented contract of the intrinsic. The intrinsic is unstable so we can change the contract, but I'm saying the new contract is too fragile and non-local and I proposed an alternative that is IMO better.

Indeed, I get it. Perhaps we can wait until Ubcheck is adjusted again to consider the issue at MIR-opts.

RalfJung · 2024-03-17T09:33:46Z

I've opened a PR that implements the plan I laid out above: #122629.

After that lands, this PR can be resurrected, it then just needs some proper docs changes in the ub_checks intrinsic.

… r=<try> refactor check_{lang,library}_ub: use a single intrinsics This enacts the plan I laid out [here](rust-lang#122282 (comment)): use a single intrinsic, called `ub_checks` (in aniticpation of rust-lang/compiler-team#725), that just exposes the value of `debug_assertions` (consistently implemented in both codegen and the interpreter). Put the language vs library UB logic into the library. This makes it easier to do something like rust-lang#122282 in the future: that just slightly alters the semantics of `ub_checks` (making it more approximating when crates built with different flags are mixed), but it no longer affects whether these checks can happen in Miri or compile-time. The first commit just moves things around; I don't think these macros and functions belong into `intrinsics.rs` as they are not intrinsics. r? `@saethlin`

… r=<try> refactor check_{lang,library}_ub: use a single intrinsic This enacts the plan I laid out [here](rust-lang#122282 (comment)): use a single intrinsic, called `ub_checks` (in aniticpation of rust-lang/compiler-team#725), that just exposes the value of `debug_assertions` (consistently implemented in both codegen and the interpreter). Put the language vs library UB logic into the library. This makes it easier to do something like rust-lang#122282 in the future: that just slightly alters the semantics of `ub_checks` (making it more approximating when crates built with different flags are mixed), but it no longer affects whether these checks can happen in Miri or compile-time. The first commit just moves things around; I don't think these macros and functions belong into `intrinsics.rs` as they are not intrinsics. r? `@saethlin`

… r=saethlin refactor check_{lang,library}_ub: use a single intrinsic This enacts the plan I laid out [here](rust-lang#122282 (comment)): use a single intrinsic, called `ub_checks` (in aniticpation of rust-lang/compiler-team#725), that just exposes the value of `debug_assertions` (consistently implemented in both codegen and the interpreter). Put the language vs library UB logic into the library. This makes it easier to do something like rust-lang#122282 in the future: that just slightly alters the semantics of `ub_checks` (making it more approximating when crates built with different flags are mixed), but it no longer affects whether these checks can happen in Miri or compile-time. The first commit just moves things around; I don't think these macros and functions belong into `intrinsics.rs` as they are not intrinsics. r? `@saethlin`

DianQK · 2024-03-24T04:19:22Z

I've tried to update the document based on the new changes.

DianQK · 2024-03-24T04:19:39Z

@bors try @rust-timer queue

rust-timer · 2024-03-24T04:19:41Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

DianQK · 2024-03-24T04:34:37Z

I can't reopen this PR because GitHub doesn't allow it. It looks like this try also can't find the new PR changes. Is there a way to cancel it?

The new PR is #122975.

refactor check_{lang,library}_ub: use a single intrinsic This enacts the plan I laid out [here](rust-lang/rust#122282 (comment)): use a single intrinsic, called `ub_checks` (in aniticpation of rust-lang/compiler-team#725), that just exposes the value of `debug_assertions` (consistently implemented in both codegen and the interpreter). Put the language vs library UB logic into the library. This makes it easier to do something like rust-lang/rust#122282 in the future: that just slightly alters the semantics of `ub_checks` (making it more approximating when crates built with different flags are mixed), but it no longer affects whether these checks can happen in Miri or compile-time. The first commit just moves things around; I don't think these macros and functions belong into `intrinsics.rs` as they are not intrinsics. r? `@saethlin`

RalfJung · 2024-03-24T08:14:52Z

(If you close a PR and then push to a branch, you can't reopen. You can only reopen when the branch is at the exact same commit as when it was closed.)

DianQK · 2024-03-24T11:21:02Z

(If you close a PR and then push to a branch, you can't reopen. You can only reopen when the branch is at the exact same commit as when it was closed.)

I see. Yes, this is the proper procedure.

… r=saethlin refactor check_{lang,library}_ub: use a single intrinsic This enacts the plan I laid out [here](rust-lang#122282 (comment)): use a single intrinsic, called `ub_checks` (in aniticpation of rust-lang/compiler-team#725), that just exposes the value of `debug_assertions` (consistently implemented in both codegen and the interpreter). Put the language vs library UB logic into the library. This makes it easier to do something like rust-lang#122282 in the future: that just slightly alters the semantics of `ub_checks` (making it more approximating when crates built with different flags are mixed), but it no longer affects whether these checks can happen in Miri or compile-time. The first commit just moves things around; I don't think these macros and functions belong into `intrinsics.rs` as they are not intrinsics. r? `@saethlin`

refactor check_{lang,library}_ub: use a single intrinsic This enacts the plan I laid out [here](rust-lang/rust#122282 (comment)): use a single intrinsic, called `ub_checks` (in aniticpation of rust-lang/compiler-team#725), that just exposes the value of `debug_assertions` (consistently implemented in both codegen and the interpreter). Put the language vs library UB logic into the library. This makes it easier to do something like rust-lang/rust#122282 in the future: that just slightly alters the semantics of `ub_checks` (making it more approximating when crates built with different flags are mixed), but it no longer affects whether these checks can happen in Miri or compile-time. The first commit just moves things around; I don't think these macros and functions belong into `intrinsics.rs` as they are not intrinsics. r? `@saethlin`

rustbot assigned saethlin Mar 10, 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 10, 2024

matthiaskrgr reviewed Mar 10, 2024

View reviewed changes

compiler/rustc_feature/src/builtin_attrs.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

DianQK force-pushed the simplify_ub_check branch from 1fdfb9b to 1357144 Compare March 10, 2024 11:17

clubby789 reviewed Mar 10, 2024

View reviewed changes

compiler/rustc_feature/src/unstable.rs Outdated Show resolved Hide resolved

DianQK force-pushed the simplify_ub_check branch from 1357144 to e6474b8 Compare March 10, 2024 12:14

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 10, 2024

This comment has been minimized.

Sign in to view

saethlin reviewed Mar 10, 2024

View reviewed changes

compiler/rustc_mir_transform/src/instsimplify.rs Outdated Show resolved Hide resolved

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 10, 2024

saethlin reviewed Mar 11, 2024

View reviewed changes

compiler/rustc_feature/src/builtin_attrs.rs Outdated Show resolved Hide resolved

saethlin reviewed Mar 11, 2024

View reviewed changes

compiler/rustc_mir_transform/src/instsimplify.rs Outdated Show resolved Hide resolved

saethlin reviewed Mar 11, 2024

View reviewed changes

compiler/rustc_mir_transform/src/instsimplify.rs Outdated Show resolved Hide resolved

DianQK force-pushed the simplify_ub_check branch from e6474b8 to e70f5e6 Compare March 11, 2024 13:48

DianQK closed this Mar 16, 2024

DianQK deleted the simplify_ub_check branch March 16, 2024 02:10

saethlin mentioned this pull request Mar 16, 2024

Discriminant update fails to optimize #122600

Open

RalfJung mentioned this pull request Mar 17, 2024

refactor check_{lang,library}_ub: use a single intrinsic #122629

Merged

DianQK restored the simplify_ub_check branch March 24, 2024 01:27

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

DianQK mentioned this pull request Mar 28, 2024

Eliminate UbChecks for non-standard libraries #122975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate `UbCheck` for non-standard libraries #122282

Eliminate `UbCheck` for non-standard libraries #122282

DianQK commented Mar 10, 2024 •

edited

rustbot commented Mar 10, 2024

This comment has been minimized.

DianQK commented Mar 10, 2024

This comment has been minimized.

bors commented Mar 10, 2024

bors commented Mar 10, 2024

This comment has been minimized.

saethlin commented Mar 10, 2024 •

edited

rust-timer commented Mar 10, 2024

DianQK commented Mar 11, 2024

saethlin commented Mar 11, 2024 •

edited

DianQK commented Mar 11, 2024

saethlin commented Mar 11, 2024

saethlin commented Mar 15, 2024 •

edited

RalfJung commented Mar 15, 2024 •

edited

DianQK commented Mar 15, 2024

RalfJung commented Mar 15, 2024 •

edited

DianQK commented Mar 15, 2024

RalfJung commented Mar 15, 2024 •

edited

DianQK commented Mar 16, 2024

RalfJung commented Mar 17, 2024

DianQK commented Mar 24, 2024

DianQK commented Mar 24, 2024

rust-timer commented Mar 24, 2024

DianQK commented Mar 24, 2024 •

edited

RalfJung commented Mar 24, 2024

DianQK commented Mar 24, 2024

Eliminate UbCheck for non-standard libraries #122282

Eliminate UbCheck for non-standard libraries #122282

Conversation

DianQK commented Mar 10, 2024 • edited

rustbot commented Mar 10, 2024

This comment has been minimized.

DianQK commented Mar 10, 2024

This comment has been minimized.

bors commented Mar 10, 2024

bors commented Mar 10, 2024

This comment has been minimized.

saethlin commented Mar 10, 2024 • edited

rust-timer commented Mar 10, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

DianQK commented Mar 11, 2024

saethlin commented Mar 11, 2024 • edited

DianQK commented Mar 11, 2024

saethlin commented Mar 11, 2024

saethlin commented Mar 15, 2024 • edited

RalfJung commented Mar 15, 2024 • edited

DianQK commented Mar 15, 2024

RalfJung commented Mar 15, 2024 • edited

DianQK commented Mar 15, 2024

RalfJung commented Mar 15, 2024 • edited

DianQK commented Mar 16, 2024

RalfJung commented Mar 17, 2024

DianQK commented Mar 24, 2024

DianQK commented Mar 24, 2024

rust-timer commented Mar 24, 2024

DianQK commented Mar 24, 2024 • edited

RalfJung commented Mar 24, 2024

DianQK commented Mar 24, 2024

Eliminate `UbCheck` for non-standard libraries #122282

Eliminate `UbCheck` for non-standard libraries #122282

DianQK commented Mar 10, 2024 •

edited

saethlin commented Mar 10, 2024 •

edited

saethlin commented Mar 11, 2024 •

edited

saethlin commented Mar 15, 2024 •

edited

RalfJung commented Mar 15, 2024 •

edited

RalfJung commented Mar 15, 2024 •

edited

RalfJung commented Mar 15, 2024 •

edited

DianQK commented Mar 24, 2024 •

edited