Remove fixme about LLVM basic block naming by erikdesjardins · Pull Request #122567 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation18 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This may be a small perf win.
Originally, this PR implemented the fixme, but it didn't have any measurable perf improvement.
r? @ghost
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
This comment has been minimized.
bors added a commit to rust-lang-ci/rust that referenced this pull request
Avoid naming LLVM basic blocks when fewer_names is true
This may be a small perf win.
r? @ghost
| 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() { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't fewer_names off by default, so need to revert this condition to actually test perf?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
☀️ Try build successful - checks-actions
Build commit: 6034f04 (6034f04447872472a7bd3b4c9f9bc006d2a2d3da)
This comment has been minimized.
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%)
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.
This comment has been minimized.
bors added a commit to rust-lang-ci/rust that referenced this pull request
Avoid naming LLVM basic blocks when fewer_names is true
This may be a small perf win.
r? @ghost
☀️ Try build successful - checks-actions
Build commit: cc5ecf7 (cc5ecf7139c96137de577412acd868a904df193a)
This comment has been minimized.
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.
Avoid naming LLVM basic blocks when Remove fixme about LLVM basic block namingfewer_names is true
No measurable change, there doesn't even seem to be a trend below the noise floor. I'll just delete the fixme.
r? compiler
📌 Commit a7d4258 has been approved by oli-obk
It is now in the queue for this repository.
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
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
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
This was referenced
Mar 18, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 8 pull requests
Successful merges:
- rust-lang#122480 (Avoid various uses of
Option<Span>in favor of usingDUMMY_SPin the few cases that usedNone) - 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
MayContainYieldPointformat args visitor) - rust-lang#122683 (add missing test: expected paren or brace in macro)
- rust-lang#122689 (Add missing
try_visitcalls in visitors.)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
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
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.