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

Evaluate std::fmt::Arguments::new_const() during Compile Time #131663

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

veera-sivarajan
Copy link
Contributor

@veera-sivarajan veera-sivarajan commented Oct 13, 2024

Fixes #128709

This PR aims to optimize calls to string formating macros without any arguments by evaluating std::fmt::Arguments::new_const() in a const context.

Currently,
println!("hola") compiles to std::io::_print(std::fmt::Arguments::new_const(&["hola\n"])).

With this PR,
println!("hola") compiles to std::io::_print(const { std::fmt::Arguments::new_const(&["hola\n"]) }).

This is accomplished in two steps:

  1. Const stabilize std::fmt::Arguments::new_const().
  2. Wrap calls to std::fmt::Arguments::new_const() in an inline const block when lowering the AST to HIR.

This reduces the generated code to a memcpy instead of multiple getelementptr and store instructions even with -C no-prepopulate-passes -C opt-level=0. Godbolt for code comparison: https://rust.godbolt.org/z/P7Px7de6c

This is a safe and sound transformation because std::fmt::Arguments::new_const() is a trivial constructor function taking a slice containing a 'static string literal as input.

CC #99012

@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2024

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Oct 13, 2024
@@ -333,7 +333,7 @@ pub struct Arguments<'a> {
#[unstable(feature = "fmt_internals", issue = "none")]
impl<'a> Arguments<'a> {
#[inline]
#[rustc_const_unstable(feature = "const_fmt_arguments_new", issue = "none")]
#[rustc_const_stable(feature = "const_fmt_arguments_new", since = "CURRENT_RUSTC_VERSION")]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this warrants a separate stabilization PR because it's an internal only API behind [feature(fmt_internals)].

@Kobzol
Copy link
Contributor

Kobzol commented Oct 13, 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 Oct 13, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 13, 2024
…<try>

Evaluate `std::fmt::Arguments::new_const()` during Compile Time

Fixes rust-lang#128709

This PR aims to optimize calls to string formating macros without any arguments by evaluating `std::fmt::Arguments::new_const()` in a const context.

Currently,
`println!("hola")` compiles to `std::io::_print(std::fmt::Arguments::new_const(&["hola\n"]))`.

With this PR,
`println!("hola")` compiles to `std::io::_print(const { std::fmt::Arguments::new_const(&["hola\n"]) })`.

This is accomplished in two steps:

1.  Const stabilize `std::fmt::Arguments::new_const()`.
2.  Wrap calls to `std::fmt::Arguments::new_const()` in an inline const block when lowering the AST to HIR.

This reduces the generated code to a `memcpy` instead of multiple `getelementptr` and `store` instructions even with `-C no-prepopulate-passes -C opt-level=0`. Godbolt for code comparison: https://rust.godbolt.org/z/P7Px7de6c

This is a safe and sound transformation because `std::fmt::Arguments::new_const()` is a trivial constructor function taking a slice containing a `'static` string literal as input.

CC rust-lang#99012
@bors
Copy link
Contributor

bors commented Oct 13, 2024

⌛ Trying commit 7638733 with merge ed038de...

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2024

Some changes occurred in coverage tests.

cc @Zalathar

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Oct 14, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs 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
Status: In progress
Development

Successfully merging this pull request may close these issues.

generated call to core::fmt::Arguments::new_const fails to constant fold
7 participants