Remove fixme about LLVM basic block naming #122567

erikdesjardins · 2024-03-15T20:04:50Z

~~This may be a small perf win.~~

Originally, this PR implemented the fixme, but it didn't have any measurable perf improvement.

r? @ghost

lqd · 2024-03-15T20:58:06Z

@bors try @rust-timer queue

bors · 2024-03-15T20:59:15Z

⌛ Trying commit 32fb9cd with merge 6034f04...

Avoid naming LLVM basic blocks when `fewer_names` is true This may be a small perf win. r? `@ghost`

klensy · 2024-03-15T21:02:25Z

compiler/rustc_codegen_llvm/src/builder.rs

-            let name = SmallCStr::new(name);
-            llvm::LLVMAppendBasicBlockInContext(cx.llcx, llfn, name.as_ptr())
+            let c_str_name;
+            let name_ptr = if cx.tcx.sess.fewer_names() {


Isn't fewer_names off by default, so need to revert this condition to actually test perf?

No, it's always enabled under normal circumstances. It's only set to false if you pass --emit=llvm-ir, or similar things that dump IR or assembly.

bors · 2024-03-15T22:33:38Z

☀️ Try build successful - checks-actions
Build commit: 6034f04 (6034f04447872472a7bd3b4c9f9bc006d2a2d3da)

rust-timer · 2024-03-16T00:59:23Z

Finished benchmarking commit (6034f04): 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)	1.9%	[1.7%, 2.0%]	3
Regressions ❌ (secondary)	2.7%	[2.2%, 3.2%]	14
Improvements ✅ (primary)	-2.7%	[-2.7%, -2.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[-2.7%, 2.0%]	4

Cycles

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

Binary size

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

Bootstrap: 669.355s -> 671.732s (0.36%)
Artifact size: 311.47 MiB -> 311.44 MiB (-0.01%)

erikdesjardins · 2024-03-16T02:06:50Z

No effect; let's try again with the Rust side string formatting also avoided.

The latest commit has some small effect locally, but I'm not sure if it'll be significant:

> -1,519,050  <rustc_codegen_ssa::mir::FunctionCx<rustc_codegen_llvm::builder::Builder>>::try_llbb:???
> -1,509,872  core::fmt::write:???
>   -841,568  alloc::fmt::format::format_inner:???

There simply might not be enough bbs for this to matter.

Kobzol · 2024-03-16T06:59:43Z

@bors try @rust-timer queue

bors · 2024-03-16T07:00:55Z

⌛ Trying commit 4831dce with merge cc5ecf7...

Avoid naming LLVM basic blocks when `fewer_names` is true This may be a small perf win. r? `@ghost`

bors · 2024-03-16T08:34:02Z

☀️ Try build successful - checks-actions
Build commit: cc5ecf7 (cc5ecf7139c96137de577412acd868a904df193a)

rust-timer · 2024-03-16T09:48:35Z

Finished benchmarking commit (cc5ecf7): 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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-	-	0

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

Binary size

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

Bootstrap: 670.527s -> 670.577s (0.01%)
Artifact size: 311.59 MiB -> 311.54 MiB (-0.02%)

Avoiding the naming didn't have any meaningful perf impact.

erikdesjardins · 2024-03-16T15:13:06Z

No measurable change, there doesn't even seem to be a trend below the noise floor. I'll just delete the fixme.

r? compiler

oli-obk · 2024-03-18T09:37:02Z

@bors r+ rollup

bors · 2024-03-18T09:37:04Z

📌 Commit a7d4258 has been approved by oli-obk

It is now in the queue for this repository.

Remove fixme about LLVM basic block naming ~This may be a small perf win.~ Originally, this PR implemented the fixme, but it didn't have any measurable perf improvement. r? `@ghost`

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#122480 (Avoid various uses of `Option<Span>` in favor of using `DUMMY_SP` in the few cases that used `None`) - rust-lang#122567 (Remove fixme about LLVM basic block naming) - rust-lang#122588 (less useless filter calls in imported_source_file) - rust-lang#122647 (add_retag: ensure box-to-raw-ptr casts are preserved for Miri) - rust-lang#122649 (Update the minimum external LLVM to 17) - rust-lang#122680 (Do not eat nested expressions' results in `MayContainYieldPoint` format args visitor) - rust-lang#122683 (add missing test: expected paren or brace in macro) - rust-lang#122689 (Add missing `try_visit` calls in visitors.) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#122567 - erikdesjardins:noname, r=oli-obk Remove fixme about LLVM basic block naming ~This may be a small perf win.~ Originally, this PR implemented the fixme, but it didn't have any measurable perf improvement. r? ``@ghost``

avoid naming LLVM basic blocks when fewer_names is true

129b5e4

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 Mar 15, 2024

erikdesjardins closed this Mar 15, 2024

erikdesjardins reopened this Mar 15, 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 Mar 15, 2024

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

Auto merge of rust-lang#122567 - erikdesjardins:noname, r=<try>

6034f04

Avoid naming LLVM basic blocks when `fewer_names` is true This may be a small perf win. r? `@ghost`

klensy reviewed Mar 15, 2024

View reviewed changes

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 Mar 16, 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 Mar 16, 2024

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

Auto merge of rust-lang#122567 - erikdesjardins:noname, r=<try>

cc5ecf7

Avoid naming LLVM basic blocks when `fewer_names` is true This may be a small perf win. 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 Mar 16, 2024

revert changes and just delete the fixme

a7d4258

Avoiding the naming didn't have any meaningful perf impact.

erikdesjardins force-pushed the noname branch from 4831dce to a7d4258 Compare March 16, 2024 15:12

erikdesjardins changed the title ~~Avoid naming LLVM basic blocks when fewer_names is true~~ Remove fixme about LLVM basic block naming Mar 16, 2024

erikdesjardins marked this pull request as ready for review March 16, 2024 15:13

rustbot assigned oli-obk Mar 16, 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 18, 2024

This was referenced Mar 18, 2024

Rollup of 8 pull requests #122688

Closed

Rollup of 8 pull requests #122690

Merged

bors merged commit 87b5679 into rust-lang:master Mar 18, 2024
11 checks passed

rustbot added this to the 1.79.0 milestone Mar 18, 2024

erikdesjardins deleted the noname branch March 18, 2024 22:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove fixme about LLVM basic block naming #122567

Remove fixme about LLVM basic block naming #122567

erikdesjardins commented Mar 15, 2024 •

edited

Loading

lqd commented Mar 15, 2024

This comment has been minimized.

bors commented Mar 15, 2024

klensy Mar 15, 2024

erikdesjardins Mar 15, 2024

bors commented Mar 15, 2024

This comment has been minimized.

rust-timer commented Mar 16, 2024

erikdesjardins commented Mar 16, 2024 •

edited

Loading

Kobzol commented Mar 16, 2024

This comment has been minimized.

bors commented Mar 16, 2024

bors commented Mar 16, 2024

This comment has been minimized.

rust-timer commented Mar 16, 2024

erikdesjardins commented Mar 16, 2024 •

edited

Loading

oli-obk commented Mar 18, 2024

bors commented Mar 18, 2024

Remove fixme about LLVM basic block naming #122567

Remove fixme about LLVM basic block naming #122567

Conversation

erikdesjardins commented Mar 15, 2024 • edited Loading

lqd commented Mar 15, 2024

This comment has been minimized.

bors commented Mar 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 15, 2024

This comment has been minimized.

rust-timer commented Mar 16, 2024

Overall result: no relevant changes - no action needed

erikdesjardins commented Mar 16, 2024 • edited Loading

Kobzol commented Mar 16, 2024

This comment has been minimized.

bors commented Mar 16, 2024

bors commented Mar 16, 2024

This comment has been minimized.

rust-timer commented Mar 16, 2024

Overall result: no relevant changes - no action needed

erikdesjardins commented Mar 16, 2024 • edited Loading

oli-obk commented Mar 18, 2024

bors commented Mar 18, 2024

erikdesjardins commented Mar 15, 2024 •

edited

Loading

erikdesjardins commented Mar 16, 2024 •

edited

Loading

erikdesjardins commented Mar 16, 2024 •

edited

Loading