Eliminate UbCheck for non-standard libraries

[DianQK]

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]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[DianQK]

@bors try @rust-timer queue

[bors]

⌛ Trying commit e6474b8 with merge acc30d0...

[bors]

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

[saethlin]

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]

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]

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

[saethlin]

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]

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]

Very nice.

@bors r+

[saethlin]

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]

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]

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]

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]

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]

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]

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]

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.

[DianQK]

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

[DianQK]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[DianQK]

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.

[RalfJung]

(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]

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