Add support for leaf function frame pointer elimination #86652
Conversation
|
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
|
rust-highfive
added
the
S-waiting-on-review
|
I'm not qualified to evaluate the code changes, but they look correct to me. However, I think the title of the PR is opposite to your intent, because it seems that you're wanting to have support for emitting frame pointers only for non-leaf functions, instead of eliminating frame pointers only for non-leaf functions. (This is understandable, because the previous code spoke in terms of eliminating frame pointers.) |
nagisa
changed the title
nagisa
force-pushed
the
nagisa/non-leaf-fp
branch
from
257c9e6
to
04e25cb
Compare
|
There is no test to show that aarch64_apple_darwin gets NonLeaf and i686_apple_darwin get Always? |
nagisa
force-pushed
the
nagisa/non-leaf-fp
branch
2 times, most recently
from
ad52a0c
to
da4642b
Compare
|
Looks like this comment
was addressed (but with r=me with the nit #86652 (comment) addressed. |
petrochenkov
added
S-waiting-on-author
This PR adds ability for the target specifications to specify frame pointer emission type that's not just “always” or “whatever cg decides”. In particular there's a new mode that allows omission of the frame pointer for leaf functions (those that don't call any other functions). We then set this new mode for Aarch64-based Apple targets. Fixes rust-lang#86196
nagisa
force-pushed
the
nagisa/non-leaf-fp
branch
from
da4642b
to
9b67cba
Compare
|
@bors r=petrochenkov |
|
📌 Commit 9b67cba has been approved by |
bors
added
S-waiting-on-bors
…laumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#86558 (Add suggestions for "undefined reference" link errors) - rust-lang#86616 (rustc_span: Explicitly handle crates that differ from package names) - rust-lang#86652 (Add support for leaf function frame pointer elimination) - rust-lang#86666 (Fix misleading "impl Trait" error) - rust-lang#86762 (mailmap: Add my work email address) - rust-lang#86773 (Enable the tests developed with rust-lang#86594) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This option changes from boolean to three options, "always", "non-leaf" and "may-omit" in Rust 1.55 [0]. [0]: rust-lang/rust#86652 Signed-off-by: Gary Guo <gary@garyguo.net>
| pub fn must_not_eliminate_frame_pointers(&self) -> bool { | ||
| // "mcount" function relies on stack pointer. | ||
| // See <https://sourceware.org/binutils/docs/gprof/Implementation.html>. | ||
| if self.instrument_mcount() { | ||
| true | ||
| } else if let Some(x) = self.opts.cg.force_frame_pointers { | ||
| x | ||
| } else { | ||
| !self.target.eliminate_frame_pointer | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
In this PR, the behavior here seems to have been reversed: previously, the target decided the default frame pointer disposition last, and the CLI argument took precedence...
| cstr!("all"), | ||
| ); | ||
| pub fn set_frame_pointer_type(cx: &CodegenCx<'ll, '_>, llfn: &'ll Value) { | ||
| let mut fp = cx.sess().target.frame_pointer; |
There was a problem hiding this comment.
...but in this place, the behavior is now to take the target frame pointer first...
| if cx.sess().instrument_mcount() || matches!(cx.sess().opts.cg.force_frame_pointers, Some(true)) | ||
| { | ||
| fp = FramePointer::Always; |
There was a problem hiding this comment.
...and only consult the CLI's opinion if it demands them.
This PR adds ability for the target specifications to specify frame
pointer emission type that's not just “always” or “whatever cg decides”.
In particular there's a new mode that allows omission of the frame
pointer for leaf functions (those that don't call any other functions).
We then set this new mode for Aarch64-based Apple targets.
Fixes #86196