Skip to content

Add -Zremark-dir unstable flag to write LLVM optimization remarks to YAML #113040

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

Merged
merged 1 commit into from
Jul 2, 2023

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Jun 25, 2023

This PR adds an option for rustc to emit LLVM optimization remarks to a set of YAML files, which can then be digested by existing tools, like https://github.com/OfekShilon/optview2. When -Cremark-dir is passed, and remarks are enabled (-Cremark=all), the remarks will be now written to the specified directory, instead of being printed to standard error output. The files are named based on the CGU from which they are being generated.

Currently, the remarks are written using the LLVM streaming machinery, directly in the diagnostics handler. It seemed easier than going back to Rust and then form there back to C++ to use the streamer from the diagnostics handler. But there are many ways to implement this, of course, so I'm open to suggestions :)

I included some comments with questions into the code. Also, I'm not sure how to test this.

r? @tmiasko

@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. labels Jun 25, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jun 25, 2023

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

@Kobzol Kobzol force-pushed the llvm-remark-streamer branch from 6d1171d to 293bd10 Compare June 25, 2023 22:18
@bors
Copy link
Collaborator

bors commented Jun 28, 2023

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

@Kobzol Kobzol changed the title Add -Cremark-dir to write LLVM optimization remarks to YAML Add -Zremark-dir unstable flag to write LLVM optimization remarks to YAML Jun 28, 2023
@Kobzol Kobzol force-pushed the llvm-remark-streamer branch from 293bd10 to adfcae2 Compare June 28, 2023 14:50
@Kobzol
Copy link
Contributor Author

Kobzol commented Jun 28, 2023

Rebased and turned the flag into an unstable one. The files are now also split based on the "section" of codegen that is being performed.

@tmiasko
Copy link
Contributor

tmiasko commented Jun 28, 2023

As for testing maybe a run-make test similar to the existing ui test optimization-remark.rs? I would be nice to test filtering as well.

There seems to be a an additional pre-existing issue: the diagnostic handler is not configured at all during post-link ThinLTO optimization phase.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jun 29, 2023

There seems to be a an additional pre-existing issue: the diagnostic handler is not configured at all during post-link ThinLTO optimization phase.

Oh, so that is not on purpose? I was wondering why is there only fat LTO 😅

@Kobzol
Copy link
Contributor Author

Kobzol commented Jun 29, 2023

I added a simple test, let me know if it's any good or if I should extend it :)

@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 1, 2023

Thanks for the review, I tried to improve error reporting in general.

@tmiasko
Copy link
Contributor

tmiasko commented Jul 1, 2023

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 1, 2023

📌 Commit fc61ad3 has been approved by tmiasko

It is now in the queue for this repository.

@bors 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 Jul 1, 2023
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jul 2, 2023
…asko

Add `-Zremark-dir` unstable flag to write LLVM optimization remarks to YAML

This PR adds an option for `rustc` to emit LLVM optimization remarks to a set of YAML files, which can then be digested by existing tools, like https://github.com/OfekShilon/optview2. When `-Cremark-dir` is passed, and remarks are enabled (`-Cremark=all`), the remarks will be now written to the specified directory, **instead** of being printed to standard error output.  The files are named based on the CGU from which they are being generated.

Currently, the remarks are written using the LLVM streaming machinery, directly in the diagnostics handler. It seemed easier than going back to Rust and then form there back to C++ to use the streamer from the diagnostics handler. But there are many ways to implement this, of course, so I'm open to suggestions :)

I included some comments with questions into the code. Also, I'm not sure how to test this.

r? `@tmiasko`
@matthiaskrgr
Copy link
Member

@bors r-
#113255 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 2, 2023
@tmiasko
Copy link
Contributor

tmiasko commented Jul 2, 2023

@bors delegate=Kobzol

@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 2, 2023

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 2, 2023
@Kobzol Kobzol force-pushed the llvm-remark-streamer branch from 79a1866 to 62728c7 Compare July 2, 2023 11:41
@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 2, 2023

@bors r=@tmiasko

@bors
Copy link
Collaborator

bors commented Jul 2, 2023

📌 Commit 62728c7 has been approved by tmiasko

It is now in the queue for this repository.

@bors 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 Jul 2, 2023
@workingjubilee
Copy link
Member

@bors rollup=iffy

@bors
Copy link
Collaborator

bors commented Jul 2, 2023

⌛ Testing commit 62728c7 with merge 131a036...

@bors
Copy link
Collaborator

bors commented Jul 2, 2023

☀️ Test successful - checks-actions
Approved by: tmiasko
Pushing 131a036 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 2, 2023
@bors bors merged commit 131a036 into rust-lang:master Jul 2, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jul 2, 2023
@Kobzol Kobzol deleted the llvm-remark-streamer branch July 2, 2023 15:15
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (131a036): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -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.0% [0.7%, 1.3%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.4% [-0.4%, -0.4%] 2
Improvements ✅
(secondary)
-3.2% [-3.2%, -3.2%] 1
All ❌✅ (primary) 0.3% [-0.4%, 1.3%] 4

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.7% [-0.7%, -0.7%] 1
Improvements ✅
(secondary)
-4.3% [-5.3%, -3.2%] 6
All ❌✅ (primary) -0.7% [-0.7%, -0.7%] 1

Binary size

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

Bootstrap: 662.06s -> 661.354s (-0.11%)

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2023
Add hotness data to LLVM remarks

Slight improvement of rust-lang#113040. This makes sure that if PGO is used, remarks generated using `-Zremark-dir` will include the `Hotness` attribute.

r? `@tmiasko`
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Aug 10, 2023
Add hotness data to LLVM remarks

Slight improvement of rust-lang/rust#113040. This makes sure that if PGO is used, remarks generated using `-Zremark-dir` will include the `Hotness` attribute.

r? `@tmiasko`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
Add hotness data to LLVM remarks

Slight improvement of rust-lang/rust#113040. This makes sure that if PGO is used, remarks generated using `-Zremark-dir` will include the `Hotness` attribute.

r? `@tmiasko`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
Add hotness data to LLVM remarks

Slight improvement of rust-lang/rust#113040. This makes sure that if PGO is used, remarks generated using `-Zremark-dir` will include the `Hotness` attribute.

r? `@tmiasko`
@OfekShilon
Copy link

@Kobzol Thank you for this! I hope to incorporate this into compiler-explorer: compiler-explorer/compiler-explorer#7525

Did this make it into official releases? Is there a non--Z switch for it, and if so - from which version?

@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 27, 2025

It wasn't stabilized yet, and I'm not sure if we should stabilize it, tbh, it's quite niche. I created a Cargo tool for automating the usage of the flag and visualizing the results (https://github.com/Kobzol/cargo-remark).

For using it in compiler explorer, you'd need to use a nightly Rust compiler.

@OfekShilon
Copy link

@Kobzol this is now in compiler-explorer for rustc nightly: https://www.compiler-explorer.com/z/b8adnsz5q
Note the example link checks all filters, by default only missed remarks are displayed.

Thank you for this work!

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 15, 2025

That's very cool, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.