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

General optimizations #275

Merged
merged 2 commits into from
Dec 7, 2023
Merged

General optimizations #275

merged 2 commits into from
Dec 7, 2023

Conversation

Peppe289
Copy link
Contributor

@Peppe289 Peppe289 commented Dec 7, 2023

Optimizations

Let's move on to things related to the code:

  • If we put the static function it comes out cleaner and the assembly is happy too (yes, I know that perhaps it could be the compiler that optimizes it but it's a convention);
  • Why allocate stack memory when you don't need it? instead use inline and where possible also macros.

Unnecessary

  • It's a great code, don't let the switch() warnings spoil the perfection...
  • We are at version 3.28 of cmake... let's raise the minimum required to remove the warning.

I tested it and it seems to work fine.

Thank's for your job.
Best regards Peppe289

There is no need to have the address on the outside of the module.

Signed-off-by: Peppe289 <gsperanza204@protonmail.com>
Copy link
Owner

@FlyGoat FlyGoat left a comment

Choose a reason for hiding this comment

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

Commit 1 to 3 LGTM, thanks for your work!
For commit 4 perhaps we should leave this kind of work for LTO :-)

lib/osdep_linux.c Outdated Show resolved Hide resolved
@FlyGoat
Copy link
Owner

FlyGoat commented Dec 7, 2023

It reminds me that I was intended to enable -fvisibility=hidden for RyzenAdj :-)

@FlyGoat FlyGoat self-assigned this Dec 7, 2023
@FlyGoat
Copy link
Owner

FlyGoat commented Dec 7, 2023

Seems like -fvisibility=hidden is working as intended for now.

@Peppe289
Copy link
Contributor Author

Peppe289 commented Dec 7, 2023

Commit 1 to 3 LGTM, thanks for your work! For commit 4 perhaps we should leave this kind of work for LTO :-)

In cmake I didn't see options for LTO by default.

@Peppe289
Copy link
Contributor Author

Peppe289 commented Dec 7, 2023

It reminds me that I was intended to enable -fvisibility=hidden for RyzenAdj :-)

yeah optimizing through the compiler is a great option.

@Peppe289
Copy link
Contributor Author

Peppe289 commented Dec 7, 2023

Well, I saw that you changed the CMake. My commit about that is no longer necessary, I will remove it.

Signed-off-by: Peppe289 <gsperanza204@protonmail.com>
@Peppe289
Copy link
Contributor Author

Peppe289 commented Dec 7, 2023

Commit dropped, and fix for conflicts. I had committed to update cmake, but it was done this morning. So it is no longer necessary.

Best regards.

@Peppe289 Peppe289 requested a review from FlyGoat December 7, 2023 13:11
@FlyGoat
Copy link
Owner

FlyGoat commented Dec 7, 2023

All LGTM, thanks!

@FlyGoat FlyGoat merged commit 3cf0734 into FlyGoat:master Dec 7, 2023
2 checks passed
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