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

⚡ Reduce code binary size #708

Open
wants to merge 12 commits into
base: develop-pros-4
Choose a base branch
from

Conversation

ion098
Copy link
Contributor

@ion098 ion098 commented Oct 24, 2024

Summary:

This PR reduces the memory footprint of PROS projects; it does this by reducing code size through the usage of Thumb mode, as well as removing unneeded linker sections.

Motivation:

Reducing memory footprint means smaller upload times.

Test Plan:

  • Check it compiles
  • Check that the kernel runs fine

@Rocky14683
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Rocky14683
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Rocky14683
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@djava
Copy link
Contributor

djava commented Oct 25, 2024

Explanation for these changes please? I'm unclear on why you're making like, any of them.

firmware/v5-common.ld Outdated Show resolved Hide resolved
src/system/unwind.c Outdated Show resolved Hide resolved
@Rocky14683
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Rocky14683
Copy link
Member

/azp run

Copy link

Pull request contains merge conflicts.

common.mk Outdated Show resolved Hide resolved
src/system/unwind.c Show resolved Hide resolved
@Rocky14683
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@SizzinSeal
Copy link
Contributor

tested, works

@Rocky14683
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Rocky14683
Copy link
Member

Download link

@SizzinSeal
Copy link
Contributor

this works, tests successful

@SizzinSeal
Copy link
Contributor

SizzinSeal commented Nov 5, 2024

Here is the size difference of the cold package when using -mthumb-interwork and not:

with -mthumb-interwork 1,326,332 bytes
without -mthumb-interwork 1,326,332 bytes

They are exactly the same size. This indicates that -mthumb-interwork has no effect on the size of the cold package.
This is supported by the GCC docs:

-mthumb-interwork
Generate code that supports calling between the ARM and Thumb instruction sets. Without this option, on pre-v5 architectures, the two instruction sets cannot be reliably used inside one program. The default is -mno-thumb-interwork, since slightly larger code is generated when -mthumb-interwork is specified. In AAPCS configurations this option is meaningless.

There are 2 points in the text that support this:

In AAPCS configurations this option is meaningless

PROS uses an AAPCS configuration

Without this option, on pre-v5 architectures, the two instruction sets cannot be reliably used inside one program

The Cortex A9 in the brain uses the "arm-v7" architecture, which is not a "pre-v5" architecture, implying that this option is not needed.

Since it has been shown PROS works without issue without the -mthumb-interwork flag, and is supported by GCC documentation and testing, the -mthumb-interwork flag should not be added back.

Remove uneeded sections, and sort sections by alignment to reduce padding
@Rocky14683
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Rocky14683
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Rocky14683
Copy link
Member

Download link

@ion098 ion098 marked this pull request as ready for review November 5, 2024 18:46
@SizzinSeal
Copy link
Contributor

@ion098 is this ready to be merged?

@ion098 ion098 changed the title ⚡ Reduce memory usage ⚡ Reduce code binary size Nov 15, 2024
@ion098
Copy link
Contributor Author

ion098 commented Nov 15, 2024

@SizzinSeal Yes, this is ready to be merged now.

@SizzinSeal
Copy link
Contributor

@Rocky14683 the download link is invalid

@Rocky14683
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants