Don't emit `expect`/`assume` in opt-level=0 #121614

clubby789 · 2024-02-25T23:00:53Z

LLVM does not make use of expect/assume calls in opt-level=0, so we can simplify IR by not emitting them in this case.

clubby789 · 2024-02-25T23:01:07Z

@bors try @rust-timer queue

Don't emit `expect`/`assume` in opt-level=0 r? `@ghost`

bors · 2024-02-25T23:02:16Z

⌛ Trying commit 45a6064 with merge bd65400...

bors · 2024-02-26T00:31:29Z

☀️ Try build successful - checks-actions
Build commit: bd65400 (bd6540009e26738ab288852d67e1ad1105f064a5)

rust-timer · 2024-02-26T02:04:18Z

Finished benchmarking commit (bd65400): comparison URL.

Overall result: no relevant changes - no 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.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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)	6.0%	[6.0%, 6.0%]	1
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-1.5%	[-1.5%, -1.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.2%	[-1.5%, 6.0%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.1%, -0.0%]	28
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.1%, -0.0%]	28

Bootstrap: 651.083s -> 651.832s (0.12%)
Artifact size: 311.22 MiB -> 311.24 MiB (0.01%)

clubby789 · 2024-02-26T02:07:22Z

Seems no perf impact, but is this still useful for the sake of emitting less unused IR?
r? compiler

saethlin · 2024-02-26T03:51:29Z

The only places we generate assume are from the transmute range assumptions in assume_scalar_range and from intrinsics::assume. The assumes in assume_scalar_range are already not generated at opt-levels 0 and 1. In a debug build, the majority of the IR from the assume will be in constructing the i1 that will be passed to llvm.assume. Plus most callers of llvm.assume go through core::hint::assert_unchecked which has assert_unsafe_precondition to check and panic in debug builds.

We only generate llvm.expect for TerminatorKind::Assert, which is pretty rare. We should probably generate more expects though.

I'm not saying whether or not this is worth doing, just that I think the above is why this has no perf impact. Previously when my niche checks prototype was based on generating new assert terminators in MIR I found that there was a perf benefit to not emitting llvm.expect in unoptimized builds.

Nadrieril · 2024-02-27T16:21:37Z

Sounds reasonable but I'm not competent. @saethlin wanna take over the review?

saethlin · 2024-02-27T16:29:14Z

Sure.
r? saethlin

saethlin · 2024-04-07T16:21:30Z

Ouch, I just found this PR while reviewing my list of PRs and issues.

We should have a bit more consistency on what we do here. For example, if we're sinking the OptLevel check into bx.assume, we should delete the check here:

rust/compiler/rustc_codegen_ssa/src/mir/statement.rs

Lines 70 to 75 in 4e431fa

    
           mir::StatementKind::Intrinsic(box NonDivergingIntrinsic::Assume(ref op)) => { 
        
               if !matches!(bx.tcx().sess.opts.optimize, OptLevel::No | OptLevel::Less) { 
        
                   let op_val = self.codegen_operand(bx, op); 
        
                   bx.assume(op_val.immediate()); 
        
               } 
        
           }

And should either delete the check here, or have evidence that sinking the check into all this code has measurable compile-time overhead (because we call all these other bx methods then don't do anything with the final bool):

rust/compiler/rustc_codegen_ssa/src/mir/rvalue.rs

Lines 326 to 333 in 4e431fa

    
           fn assume_scalar_range( 
        
               &self, 
        
               bx: &mut Bx, 
        
               imm: Bx::Value, 
        
               scalar: abi::Scalar, 
        
               backend_ty: Bx::Type, 
        
           ) { 
        
               if matches!(self.cx.sess().opts.optimize, OptLevel::No | OptLevel::Less)

Additionally, those sites check for OptLevel::No | OptLevel::Less. It's entirely unclear to me if that was based on a codegen test. Adding some tests to ensure that we emit the assumes only when they help would be nice, but really for this PR I just want consistency. Pick either just OptLevel::No or OptLevel::No | OptLevel::Less to use everywhere.

oskgo · 2024-07-11T15:12:32Z

@clubby789 Any updates on this? Thanks!

clubby789 · 2024-07-12T11:01:45Z

Deleted the first check and used OptLevel::No (presumably, there's a chance LLVM could do something in OptLevel::Yes). There are some tests already at least for the transmute value ranges

saethlin · 2024-07-12T16:26:33Z

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

@@ -332,7 +332,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
        scalar: abi::Scalar,
        backend_ty: Bx::Type,
    ) {
-        if matches!(self.cx.sess().opts.optimize, OptLevel::No | OptLevel::Less)


@scottmcm FYI this is changing; I didn't see any discussion or experiments in #109993 to support turning these off for -Copt-level=1; did you have some kind of compile time test that backed up having them off for that opt-level?

Nope; leaving them on for -O1 sounds fine by me.

(I assume we don't actually have any data for it either way.)

scottmcm · 2024-07-12T17:17:26Z

We only generate llvm.expect for TerminatorKind::Assert, which is pretty rare. We should probably generate more expects though.

I think likely/unlikely do too? Though those are unstable, so there aren't many of them.

IMHO, most of them are wrong, too. Anything that leads to a panic doesn't need an expect, because #[cold] exists to automatically mark all branches leading to that block as unlikely:

This attribute indicates that this function is rarely called. When computing edge weights, basic blocks post-dominated by a cold function call are also considered to be cold; and, thus, given low weight.

https://llvm.org/docs/LangRef.html#function-attributes

bors · 2024-07-30T06:17:32Z

☔ The latest upstream changes (presumably #128250) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2024-09-03T22:54:56Z

@clubby789
ping from triage - can you post your status on this PR? This PR has not received an update in a few months and there are merge conflics

clubby789 · 2024-09-04T13:06:26Z

Sorry - rebased and fixed up the test
@bors r=saethlin

bors · 2024-09-04T13:06:29Z

📌 Commit 5b96ae7 has been approved by saethlin

It is now in the queue for this repository.

bors · 2024-09-04T13:57:48Z

⌛ Testing commit 5b96ae7 with merge 9c64629...

Don't emit `expect`/`assume` in opt-level=0 LLVM does not make use of expect/assume calls in `opt-level=0`, so we can simplify IR by not emitting them in this case.

pietroalbini · 2024-09-04T15:05:53Z

@bors retry

Yield priority to #129960

bors · 2024-09-05T10:14:13Z

⌛ Testing commit 5b96ae7 with merge 0e79cbf...

Don't emit `expect`/`assume` in opt-level=0 LLVM does not make use of expect/assume calls in `opt-level=0`, so we can simplify IR by not emitting them in this case.

rust-log-analyzer · 2024-09-05T11:53:41Z

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Finished `release` profile [optimized] target(s) in 0.41s
##[endgroup]
[TIMING] core::build_steps::tool::ToolBuild { compiler: Compiler { stage: 1, host: x86_64-pc-windows-msvc }, target: x86_64-pc-windows-msvc, tool: "miri", path: "src/tools/miri", mode: ToolRustc, source_type: InTree, extra_features: [], allow_features: "", cargo_args: [] } -- 0.453
thread 'main' panicked at src/lib.rs:1694:17:
failed to copy `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe` to `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1\bin\miri.exe`: The process cannot access the file because it is being used by another process. (os error 32)
Build completed unsuccessfully in 0:00:09
  local time: Thu, Sep  5, 2024 11:53:28 AM
  network time: Thu, 05 Sep 2024 11:53:28 GMT
##[error]Process completed with exit code 1.

bors · 2024-09-05T11:54:19Z

💔 Test failed - checks-actions

saethlin · 2024-09-05T15:01:34Z

@bors retry #127883

bors · 2024-09-06T00:43:01Z

⌛ Testing commit 5b96ae7 with merge 54fdef7...

bors · 2024-09-06T03:05:46Z

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing 54fdef7 to master...

rust-timer · 2024-09-06T04:23:58Z

Finished benchmarking commit (54fdef7): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -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
Regressions ❌ (secondary)	4.9%	[4.9%, 4.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 0.5%)

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
Regressions ❌ (secondary)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-1.7%, -1.7%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -1.0%)

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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.0%	[-1.0%, -1.0%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 757.186s -> 755.979s (-0.16%)
Artifact size: 340.94 MiB -> 341.00 MiB (0.02%)

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. labels Feb 25, 2024

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 Feb 25, 2024

clubby789 changed the title ~~Don't codegen expect/assume in opt-level=0~~ Don't emit expect/assume in opt-level=0 Feb 25, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 25, 2024

Auto merge of rust-lang#121614 - clubby789:no-expect, r=<try>

bd65400

Don't emit `expect`/`assume` in opt-level=0 r? `@ghost`

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 26, 2024

clubby789 marked this pull request as ready for review February 26, 2024 02:07

rustbot assigned Nadrieril Feb 26, 2024

rustbot assigned saethlin and unassigned Nadrieril Feb 27, 2024

saethlin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 7, 2024

rust-cloud-vms bot force-pushed the no-expect branch from 45a6064 to 25264cf Compare July 12, 2024 11:01

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the no-expect branch from 25264cf to 766253c Compare July 12, 2024 13:54

saethlin reviewed Jul 12, 2024

View reviewed changes

Don't codegen expect in opt-level=0

5b96ae7

rust-cloud-vms bot force-pushed the no-expect branch from 766253c to 5b96ae7 Compare September 4, 2024 11:49

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 4, 2024

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 Sep 5, 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 Sep 5, 2024

bors mentioned this pull request Sep 6, 2024

Rollup of 11 pull requests #129999

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 6, 2024

bors merged commit 54fdef7 into rust-lang:master Sep 6, 2024
7 checks passed

rustbot added this to the 1.83.0 milestone Sep 6, 2024

This was referenced Sep 6, 2024

Likely unlikely fix #120370

Merged

Check for occupied niches #121174

Draft

jieyouxu added the CI-spurious-fail-msvc CI spurious failure: target env msvc label Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't emit `expect`/`assume` in opt-level=0 #121614

Don't emit `expect`/`assume` in opt-level=0 #121614

clubby789 commented Feb 25, 2024 •

edited

Loading

clubby789 commented Feb 25, 2024

This comment has been minimized.

bors commented Feb 25, 2024

bors commented Feb 26, 2024

This comment has been minimized.

rust-timer commented Feb 26, 2024

clubby789 commented Feb 26, 2024

saethlin commented Feb 26, 2024

Nadrieril commented Feb 27, 2024

saethlin commented Feb 27, 2024

saethlin commented Apr 7, 2024

oskgo commented Jul 11, 2024

clubby789 commented Jul 12, 2024

This comment has been minimized.

saethlin Jul 12, 2024

scottmcm Jul 12, 2024

scottmcm commented Jul 12, 2024

bors commented Jul 30, 2024

JohnCSimon commented Sep 3, 2024

clubby789 commented Sep 4, 2024

bors commented Sep 4, 2024

bors commented Sep 4, 2024

pietroalbini commented Sep 4, 2024

bors commented Sep 5, 2024

rust-log-analyzer commented Sep 5, 2024

bors commented Sep 5, 2024

saethlin commented Sep 5, 2024

bors commented Sep 6, 2024

bors commented Sep 6, 2024

rust-timer commented Sep 6, 2024

Don't emit expect/assume in opt-level=0 #121614

Don't emit expect/assume in opt-level=0 #121614

Conversation

clubby789 commented Feb 25, 2024 • edited Loading

clubby789 commented Feb 25, 2024

This comment has been minimized.

bors commented Feb 25, 2024

bors commented Feb 26, 2024

This comment has been minimized.

rust-timer commented Feb 26, 2024

Overall result: no relevant changes - no action needed

clubby789 commented Feb 26, 2024

saethlin commented Feb 26, 2024

Nadrieril commented Feb 27, 2024

saethlin commented Feb 27, 2024

saethlin commented Apr 7, 2024

oskgo commented Jul 11, 2024

clubby789 commented Jul 12, 2024

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Jul 12, 2024

bors commented Jul 30, 2024

JohnCSimon commented Sep 3, 2024

clubby789 commented Sep 4, 2024

bors commented Sep 4, 2024

bors commented Sep 4, 2024

pietroalbini commented Sep 4, 2024

bors commented Sep 5, 2024

rust-log-analyzer commented Sep 5, 2024

bors commented Sep 5, 2024

saethlin commented Sep 5, 2024

bors commented Sep 6, 2024

bors commented Sep 6, 2024

rust-timer commented Sep 6, 2024

Overall result: ❌ regressions - no action needed

Don't emit `expect`/`assume` in opt-level=0 #121614

Don't emit `expect`/`assume` in opt-level=0 #121614

clubby789 commented Feb 25, 2024 •

edited

Loading