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

[bazel] Move _mask_rom_interrupt_vector_c to .vectors to eliminate padding #12451

Merged
merged 1 commit into from
May 4, 2022

Conversation

alphan
Copy link
Contributor

@alphan alphan commented May 3, 2022

I had some time to look more into the size difference between the binaries built using meson vs bazel, specifically why mask_rom_exception_handler() is placed at the start of the .crt section when using bazel. This introduces extra padding since _mask_rom_interrupt_vector_c is aligned to 256 bytes.

I think, the reason why we don't have this issue in meson is because mask_rom_start.S appears before mask_rom.c. Reordering them in the srcs of opentian_rom_binary() in sw/device/silicon_creator/mask_rom/BUILD fixes the issue but would be very fragile (also not allowed by the buildifier).

I was thinking about updating the linker file and mask_rom.h to put the functions defined in c after asm definitions but realized that we can actually place _mask_rom_interrupt_vector_c into .vectors instead of .crt without any overhead since the c interrupt vector is aligned to 256 bytes as well, i.e. the comment removed in this PR doesn't seem to be accurate.

mask_rom_fpga_cw310.elf.s: before, after

This PR also reduces the size of mask_rom_fpga_cw310.bin from 31832 bytes to 31640 bytes (-192 bytes)

Related to #12069 (@timothytrippel).

Signed-off-by: Alphan Ulusoy [email protected]

@alphan alphan requested review from mcy, mundaym and cfrantz May 3, 2022 21:13
@alphan alphan closed this May 4, 2022
@alphan alphan reopened this May 4, 2022
@mcy mcy merged commit 505df8d into lowRISC:master May 4, 2022
@alphan alphan deleted the bazel-mask-rom-crt-padding branch May 4, 2022 16:28
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.

2 participants