Skip to content

Optimize function layout in hps_accel project #323

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Optimize function layout in hps_accel project #323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fba55a6
Use custom linker script in hps_accel project
j-piecuch Oct 12, 2021
a1300c6
hps_accel: include proj.mk before checking $(TARGET)
j-piecuch Oct 12, 2021
45ec30f
Add an explanatory comment to hps_accel/linker.ld
j-piecuch Oct 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ XXD := $(PYRUN) $(CFU_ROOT)/scripts/xxd.py

LD_DIR := $(BUILD_DIR)/ld
GEN_LD_DIR := $(SOC_SOFTWARE_DIR)/include/generated
LDSCRIPT := $(LD_DIR)/linker.ld
LDSCRIPT ?= $(LD_DIR)/linker.ld
LDSCRIPTS := $(LDSCRIPT) $(GEN_LD_DIR)/output_format.ld $(GEN_LD_DIR)/regions.ld

SRC_DIR := $(BUILD_DIR)/src
Expand Down
13 changes: 9 additions & 4 deletions proj/hps_accel/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ DEFINES += INCLUDE_MODEL_HPS
RUN_MENU_ITEMS=1 1 c
TEST_MENU_ITEMS=3 q

# Use a custom linker script for this project
export LDSCRIPT
LDSCRIPT = $(PROJ_DIR)/linker.ld

# Add constants defined in gateware/constants.py to C++ build
BUILD_DIR_EXTRA_DEP = $(BUILD_DIR)/src/gateware_constants.h

include ../proj.mk

# Customise arguments to Litex:
export EXTRA_LITEX_ARGS
EXTRA_LITEX_ARGS += --cpu-variant slim+cfu
Expand All @@ -67,9 +76,5 @@ ifeq '$(TARGET)' 'digilent_arty'
EXTRA_LITEX_ARGS += --sys-clk-freq 75000000
endif

# Add constants defined in gateware/constants.py to C++ build
BUILD_DIR_EXTRA_DEP = $(BUILD_DIR)/src/gateware_constants.h
include ../proj.mk

$(BUILD_DIR)/src/gateware_constants.h: $(BUILD_DIR)/src $(PROJ_DIR)/gateware/constants.py
$(PYRUN) -m gateware.constants $@
67 changes: 67 additions & 0 deletions proj/hps_accel/linker.ld
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
INCLUDE output_format.ld
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add the copyright header to this file? Just grab it from one of the others. "Copyright the CFU-Playground authors" etc.

Copy link
Author

Choose a reason for hiding this comment

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

I no longer think that providing custom linker scripts for each project is a viable solution, so this file will probably be removed. Please see my latest comments for an alternative approach.

ENTRY(_start)

__DYNAMIC = 0;

INCLUDE regions.ld


SECTIONS
{
.text :
{
_ftext = .;
*(.text.start)
/* Place the ConvPerChannel4x4() and LoadInput() functions sequentially
* to prevent one from evicting the other from the instruction cache. */
*/conv_accel.o(.text.*ConvPerChannel4x4*)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this file may have mixed spaces and tabs for indents, it would be good to fix that up for clarity.

Copy link
Author

Choose a reason for hiding this comment

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

True, but first let's decide whether this linker script should be here at all.

*/blocks.o(.text.*LoadInput*)
*(.text .stub .text.* .gnu.linkonce.t.*)
_etext = .;
} > main_ram
/*} > rom */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm why are these commented out?

Actually now that I think about it, it looks like this linker script is correct for Arty (where we run the whole CFU-Playground application from RAM) but won't work on HPS boards (where we run the application from flash)? I guess for HPS boards you have to uncomment these lines instead, right?

Is there a way we can have a single linker script that will place things in the right regions in all cases?

Copy link
Author

Choose a reason for hiding this comment

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

This linker script is essentially copy-pasted from common/ld/linker.ld, including the fragments which are commented out.
You're right, this script isn't suitable for every board. I was not aware of the platform-specific scripts in common/_hps etc.
I will write down some ideas on how to solve this in a comment outside of this conversation.


.rodata :
{
. = ALIGN(8);
_frodata = .;
*(.rodata .rodata.* .gnu.linkonce.r.*)
*(.rodata1)
*(.srodata .srodata.*)
. = ALIGN(8);
_erodata = .;
} > main_ram
/*} > rom */

.data : AT (ADDR(.rodata) + SIZEOF (.rodata))
{
. = ALIGN(8);
_fdata = .;
*(.data .data.* .gnu.linkonce.d.*)
*(.data1)
*(.ramtext .ramtext.*)
_gp = ALIGN(16);
*(.sdata .sdata.* .gnu.linkonce.s.* .sdata2 .sdata2.*)
_edata = ALIGN(16); /* Make sure _edata is >= _gp. */
} > main_ram
/*} > sram */

.bss : AT (ADDR(.data) + SIZEOF (.data))
{
. = ALIGN(16);
_fbss = .;
*(.dynsbss)
*(.sbss .sbss.* .gnu.linkonce.sb.*)
*(.scommon)
*(.dynbss)
*(.bss .bss.* .gnu.linkonce.b.*)
*(COMMON)
. = ALIGN(8);
_ebss = .;
_end = .;
} > main_ram
/*} > sram */
}

PROVIDE(_fstack = ORIGIN(main_ram) + LENGTH(main_ram) - 4);
/* PROVIDE(_fstack = ORIGIN(sram) + LENGTH(sram) - 4); */