Skip to content

Conversation

mcy
Copy link
Contributor

@mcy mcy commented Mar 4, 2020

Pull Request Overview

Disabling frame pointer accounting in rustc has a significant impact on
binary size. For ARM cores, this tends to drop text size by 2-3%, but on
RISC-V cores, we see size improvements of 9-10%. Full results below:

board            | before   | after    | Δ   | %Δ
----------------------------------------------------
imix              159.0K     154.5K     4.5K  2.83%
acd52832           61.5K      60K       1.5K  2.44%
nucleo_f446re      58.501K    57.501K   1.0K  1.71%
nucleo_f429zi      58.501K    57.501K   1.0K  1.71%
hifive1            52.5K      47.0K     5.5K  10.48%
nrf52dk            88.5K      86.0K     2.5K  2.82%
nrf52840dk        113.0K     110.0K     3.0K  2.65%
nrf52840_dongle   107.5K     105.5K     2.0K  1.86%
arty-e21           56.5K      51.0K     5.5K  9.73%
opentitan          61.9893K   56.4893K  5.5K  8.87%
hail               97.5K      94.5K     3.0K  3.08%
launchxl           62.5K      61.5K     1.0K  1.60%

This change disables frame pointers, which appear to be otherwise
unused, as a binary sliming measure.

The most egregious example is the following before/after pair:

// Before.
20000294:       1141                    addi    sp,sp,-16
20000296:       c606                    sw      ra,12(sp)
20000298:       c422                    sw      s0,8(sp)
2000029a:       0800                    addi    s0,sp,16
2000029c:       4422                    lw      s0,8(sp)
2000029e:       40b2                    lw      ra,12(sp)
200002a0:       0141                    addi    sp,sp,16
200002a2:       8082                    ret

// After.
20000294:       8082                    ret

Mind, there is the separate concern that LLVM isn't inlining and eliminating these functions, which is a separate investigation I'm undertaking.

Testing Strategy

This change is currently untested; I'm not familiar with Tock's debugging story, so I'm not sure what would be impacted by this change. This is a draft right now, and I welcome suggestions for ensuring I haven't broken anything.

TODO or Help Wanted

I'm hoping to test the waters on whether this is tenable, at least for RISC-V cores, where the impact is outsized. I don't know if frame pointers are used at all by Tock's debugging strategy, and I'm hoping to get feedback on that.

At the very least, the OpenTitan project is looking for ways to slim its use of the Tock kernel, and the 8.8% .text shrinkage is very attractive to us in production builds.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make formatall.

Disabling frame pointer accounting in rustc has a significant impact on
binary size. For ARM cores, this tends to drop size by 2-3%, but on
RISC-V cores, we see size improvements of 9-10%.

This change disables frame pointers, which appear to be otherwise
unused, as a binary sliming measure.
@ppannuto
Copy link
Member

ppannuto commented Mar 4, 2020 via email

@mcy
Copy link
Contributor Author

mcy commented Mar 4, 2020

It depends a lot. The main use is in finding where the stack frames are during debugging, and yeah, this is totally something that can be debug-only. I don't know if Tock actually takes advantage of it, and we should figure out if we'd like to or if we'd like to just discard it all.

@bradjc
Copy link
Contributor

bradjc commented Mar 4, 2020

If we aren't using this, and I don't think we are, I would say we just remove it and if someone wants to do something with it in the future we can re-evaluate including the flag on just debug builds or something.

ppannuto
ppannuto previously approved these changes Mar 4, 2020
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

I'm in agreement with @bradjc, but think we should let this one marinate for a few days in case anyone has arguments against.

@mcy
Copy link
Contributor Author

mcy commented Mar 4, 2020

I second letting this sit. I also would like to hear feelings about testing the change, and maybe restricting it to RISC-V targets (since ARM targets hardly get any benefit).

@mcy mcy changed the title DRAFT rust: disable frame points in the Tock kernel DRAFT rust: disable frame pointers in the Tock kernel Mar 4, 2020
@alistair23
Copy link
Contributor

alistair23 commented Mar 4, 2020

This seems to be the commit that enabled it for Rust: rust-lang/rust#61675

It looks like this is useful for RUST_BACKTRACE which I think we can live without. As long as GDB backtraces still work I'm happy with this change.

@mcy
Copy link
Contributor Author

mcy commented Mar 4, 2020

Unclear, that's part of what I'd like to figure out (again, underscoring that I don't have a background in how Tock does debugging). The naive frame-pointer-chasing scheme doesn't work if you don't have frame pointers, obviously, but if that's a problem we can just include frame pointers in debug builds. If you can tell me if this completely breaks your workflow, that would be great.

I'd honestly prefer to have a somewhat less blunt tool than "turn off all frame pointers". As I mentioned in the OP, I think we may need to examine other things, such as more aggressive inlining.

@labbott
Copy link
Contributor

labbott commented Mar 4, 2020

I'm traveling and my setup isn't working at the moment so I can't give this a real test until Tuesday probably. What I'd like to verify is if we still get backtraces with QEMU + gdb (attach via gdb server, break, bt)

@alistair23
Copy link
Contributor

I just did some testing on the FPGA and this patch doesn't seem to negatively affect GDB backtraces.

alistair23
alistair23 previously approved these changes Mar 5, 2020
xobs added a commit to betrusted-io/xous-kernel that referenced this pull request Mar 6, 2020
These aren't super useful to us, and they make function sizes much
larger.  Some reports are on the order of 10% larger:

tock/tock#1660

Signed-off-by: Sean Cross <sean@xobs.io>
@gendx gendx mentioned this pull request Mar 11, 2020
2 tasks
@bradjc bradjc added last-call Final review period for a pull request. needs-rebase labels Mar 11, 2020
@alevy
Copy link
Member

alevy commented Mar 12, 2020

Can we wait on getting an OK from @labbott? I trust that @alistair23's experience with the FPGA will translate to QEMU, but I'd prefer to wait an extra day or two to make sure this doesn't negatively impact development in the short term.

@labbott
Copy link
Contributor

labbott commented Mar 12, 2020

LGTM (forgot to post this earlier, I appreciate you waiting)

@alevy alevy dismissed stale reviews from alistair23 and ppannuto via 75f6e63 March 12, 2020 19:42
@alevy
Copy link
Member

alevy commented Mar 12, 2020

I resolved the remaining conflict and approved. Waiting for the build to come back green again.

@alevy
Copy link
Member

alevy commented Mar 12, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 12, 2020

Build succeeded

@bors bors bot merged commit 5e39dc2 into tock:master Mar 12, 2020
bradjc added a commit that referenced this pull request Mar 13, 2020
The reverted patch causes the kernel to panic on Hail.
@bradjc bradjc mentioned this pull request Mar 13, 2020
2 tasks
bors bot added a commit that referenced this pull request Mar 13, 2020
1688: Revert PR #1660 r=alevy a=bradjc

#1660 causes the kernel to panic on Hail.

I don't know why, but I git bisected back to this change.




### Testing Strategy

This pull request was tested by running Blink on Hail.


### TODO or Help Wanted

We should merge this and then re-evaluate #1660.


### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make formatall`.


Co-authored-by: Brad Campbell <bradjc5@gmail.com>
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 17, 2020
…=hanna-kruppe,Mark-Simulacrum

[RISC-V] Do not force frame pointers

We have been seeing some very inefficient code that went away when using
`-Cforce-frame-pointers=no`. For instance `core::ptr::drop_in_place` at
`-Oz` was compiled into a function which consisted entirely of saving
registers to the stack, then using the frame pointer to restore the same
registers (without any instructions between the prolog and epilog).

The RISC-V LLVM backend supports frame pointer elimination, so it makes
sense to allow this to happen when using Rust. It's not clear to me that
frame pointers have ever been required in the general case.

In rust-lang#61675 it was pointed out that this made reassembling
stack traces easier, which is true, but there is a code generation
option for forcing frame pointers, and I feel the default should not be
to require frame pointers, given it demonstrably makes code size worse
(around 10% in some embedded applications).

The kinds of targets mentioned in rust-lang#61675 are popular, but
should not dictate that code generation should be worse for all RISC-V
targets, especially as there is a way to use CFI information to
reconstruct the stack when the frame pointer is eliminated. It is also
a misconception that `fp` is always used for the frame pointer. `fp` is
an ABI name for `x8` (aka `s0`), and if no frame pointer is required,
`x8` may be used for other callee-saved values.

---

I am partly posting this to get feedback from @fintelia who introduced the change to require frame pointers, and @hanna-kruppe who had issues with the original PR. I would understand if we wanted to remove this setting on only a subset of RISC-V targets, but my preference would be to remove this setting everywhere.

There are more details on the code size savings seen in Tock here: tock/tock#1660
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 17, 2020
… r=hanna-kruppe,Mark-Simulacrum

[RISC-V] Do not force frame pointers

We have been seeing some very inefficient code that went away when using
`-Cforce-frame-pointers=no`. For instance `core::ptr::drop_in_place` at
`-Oz` was compiled into a function which consisted entirely of saving
registers to the stack, then using the frame pointer to restore the same
registers (without any instructions between the prolog and epilog).

The RISC-V LLVM backend supports frame pointer elimination, so it makes
sense to allow this to happen when using Rust. It's not clear to me that
frame pointers have ever been required in the general case.

In rust-lang#61675 it was pointed out that this made reassembling
stack traces easier, which is true, but there is a code generation
option for forcing frame pointers, and I feel the default should not be
to require frame pointers, given it demonstrably makes code size worse
(around 10% in some embedded applications).

The kinds of targets mentioned in rust-lang#61675 are popular, but
should not dictate that code generation should be worse for all RISC-V
targets, especially as there is a way to use CFI information to
reconstruct the stack when the frame pointer is eliminated. It is also
a misconception that `fp` is always used for the frame pointer. `fp` is
an ABI name for `x8` (aka `s0`), and if no frame pointer is required,
`x8` may be used for other callee-saved values.

---

I am partly posting this to get feedback from @fintelia who introduced the change to require frame pointers, and @hanna-kruppe who had issues with the original PR. I would understand if we wanted to remove this setting on only a subset of RISC-V targets, but my preference would be to remove this setting everywhere.

There are more details on the code size savings seen in Tock here: tock/tock#1660
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 17, 2020
… r=hanna-kruppe,Mark-Simulacrum

[RISC-V] Do not force frame pointers

We have been seeing some very inefficient code that went away when using
`-Cforce-frame-pointers=no`. For instance `core::ptr::drop_in_place` at
`-Oz` was compiled into a function which consisted entirely of saving
registers to the stack, then using the frame pointer to restore the same
registers (without any instructions between the prolog and epilog).

The RISC-V LLVM backend supports frame pointer elimination, so it makes
sense to allow this to happen when using Rust. It's not clear to me that
frame pointers have ever been required in the general case.

In rust-lang#61675 it was pointed out that this made reassembling
stack traces easier, which is true, but there is a code generation
option for forcing frame pointers, and I feel the default should not be
to require frame pointers, given it demonstrably makes code size worse
(around 10% in some embedded applications).

The kinds of targets mentioned in rust-lang#61675 are popular, but
should not dictate that code generation should be worse for all RISC-V
targets, especially as there is a way to use CFI information to
reconstruct the stack when the frame pointer is eliminated. It is also
a misconception that `fp` is always used for the frame pointer. `fp` is
an ABI name for `x8` (aka `s0`), and if no frame pointer is required,
`x8` may be used for other callee-saved values.

---

I am partly posting this to get feedback from @fintelia who introduced the change to require frame pointers, and @hanna-kruppe who had issues with the original PR. I would understand if we wanted to remove this setting on only a subset of RISC-V targets, but my preference would be to remove this setting everywhere.

There are more details on the code size savings seen in Tock here: tock/tock#1660
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
last-call Final review period for a pull request. needs-rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants