search

sifive / freedom-metal

Bare Metal Compatibility Library for the Freedom Platform
Other
152 stars 47 forks source link

Use balign 8 for 8 series, otherwise use align 8. #406

Closed lexus0 closed 3 years ago

lexus0 commented 3 years ago

The performance numbers drop for e76, and s76 with coremark, and dhrystone due to freedom-metal PR-374.

I understand it is necessary for u84 (5.6 -> 5.77 in dhrystone). Therefore, I develop a mechanism to use align or balign by recognizing RISCV_SERIES. This mechanism need another PR in freedom-e-sdk. Please review, and merge it as well.

nick-knight commented 3 years ago

My best guess is it forces a stricter alignment on an ELF section that ends up containing this (after linking).

lexus0 commented 3 years ago

I have no idea either. I just did some binary search to check which commit make performance drop for e76, and s76. This PR was not merged when I was targeting the 7 series numbers..

fuhle044 commented 3 years ago

We need to look at the map file for data layout. I suspect that the argv alignment is setting the default data sectiom alignment, which then affects the section combining alignments. Or the issue is actually in the linker script and it's doing a poor job with .sdsta/.data/.bss alignments.

paul-walmsley-sifive commented 3 years ago

Hi folks, just to follow up here: we're concerned that this patch doesn't address the root cause of the problem, and that the root cause is poorly understood. So at this time, we're not planning on merging this. @fuhle044 can you assign somebody in your group to root-cause this issue? Thanks

nick-knight commented 3 years ago

FYI @fuchingy .

fuhle044 commented 3 years ago

Already under investigation.

On Thu, Jul 1, 2021, 09:51 Paul Walmsley @.***> wrote:

Hi folks, just to follow up here: we're concerned that this patch doesn't address the root cause of the problem, and that the root cause is poorly understood. So at this time, we're not planning on merging this. @fuhle044 https://github.com/fuhle044 can you assign somebody in your group to root-cause this issue? Thanks

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sifive/freedom-metal/pull/406#issuecomment-872401392, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOB2QNSFTD3LI46XQV5DD7LTVSMIFANCNFSM47KDVSGQ .

lexus0 commented 3 years ago

hi all,

In short

I think I will create another PR for new flags to fit .balign 8 instead of having strange checking system to choose align or balign.

details

The flag mining is something like tweak the performance based on what you have. We start to tweak the performance based on .align 8. I think the performance drops from .align 8 to .balign 8 because the data layout didn't fit perfectly.

The flags that merged into master is based on .align 8. I asked @DavidSTWChen that .balign 8 is necessary for benchmark they are using, and make more sense than .align 8.

I started another round to mind the flags to fit .balign 8. They can meet the target but the flags are too long. So I'm doing some reduction work right now. I will create another PR for new flags once I finish my work. Then we can close this PR, and related PR in fesdk-private.

issuehsu commented 3 years ago

Drop this PR; we will resolve the performance drop issue with freedom-e-sdk-private PR#117