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 UbCheck for non-standard libraries #122282

Closed
wants to merge 1 commit into from

Conversation

DianQK
Copy link
Member

@DianQK DianQK commented Mar 10, 2024

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 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
@rustbot
Copy link
Collaborator

rustbot commented Mar 10, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer

This comment has been minimized.

@DianQK
Copy link
Member Author

DianQK commented Mar 10, 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 10, 2024
@bors
Copy link
Contributor

bors commented Mar 10, 2024

⌛ Trying commit e6474b8 with merge acc30d0...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 10, 2024
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
Copy link
Contributor

bors commented Mar 10, 2024

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

@rust-timer

This comment has been minimized.

@saethlin
Copy link
Member

saethlin commented Mar 10, 2024

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
Copy link
Collaborator

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

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

DianQK commented Mar 11, 2024

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

saethlin commented Mar 11, 2024

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

DianQK commented Mar 11, 2024

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

Very nice.

@bors r+

@saethlin
Copy link
Member

saethlin commented Mar 15, 2024

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

RalfJung commented Mar 15, 2024

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

DianQK commented Mar 15, 2024

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

RalfJung commented Mar 15, 2024

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

DianQK commented Mar 15, 2024

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

RalfJung commented Mar 15, 2024

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

DianQK commented Mar 16, 2024

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

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.

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2024
… 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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2024
… 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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2024
… 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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2024
… 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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2024
… 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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2024
… 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 DianQK restored the simplify_ub_check branch March 24, 2024 01:27
@DianQK
Copy link
Member Author

DianQK commented Mar 24, 2024

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

@DianQK
Copy link
Member Author

DianQK commented Mar 24, 2024

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 24, 2024
@DianQK
Copy link
Member Author

DianQK commented Mar 24, 2024

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.

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 24, 2024
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
Copy link
Member

(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
Copy link
Member Author

DianQK commented Mar 24, 2024

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

RenjiSann pushed a commit to RenjiSann/rust that referenced this pull request Mar 25, 2024
… 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`
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Apr 4, 2024
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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-perf Status: Waiting on a perf run to be completed. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants