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

Fix missing optimization for amd-fftw & upgrade to 4.2 #84

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

jeroen-mostert
Copy link
Contributor

Per #82, amd-fftw uses CFLAGS as a replacement rather than an append, so all options need to be supplied by us. This pull also upgrades to 4.2, because we can.

A vanilla build applies -O3 -fomit-frame-pointer -mtune=native -malign-double -fstrict-aliasing -fno-schedule-insns. On a modern GCC build, all of the other options are actually implied by -O3, except for two:

  • -mtune=native. We omit this because a build on a virtual machine might leave us with a badly tuned library.
  • -fno-schedule-insns. This turns off instruction scheduling. A benchmark on my end showed no improvements from this option, I think it's better to trust the compiler here. Like the other options this might be a holdover from times where GCC's optimization was worse.

This also patches build.sh so it can process configure commands with quoted options. The other spots in the file already use eval, just this one didn't. I retested the build to ensure this 1) doesn't break anything and 2) the exported CFLAGS don't end up on other builds.

@lamikr
Copy link
Owner

lamikr commented Jun 25, 2024

If you could add the to the git commit a comment line,
fixes #82
then the all the work which involved the addition of "-O3" is better referenced :-)

Another thing, did you also do a full rebuild to test that "eval" did not cause any problem? I can trigger one clean build to my desktop to test this. I think I had at some point some reason for letting the eval away from the build command execution, just don't remember what it was. It could have been some error in build scripts that does not exist anymore.

jeroen-mostert added a commit to jeroen-mostert/rocm_sdk_builder that referenced this pull request Jun 25, 2024
@jeroen-mostert
Copy link
Contributor Author

jeroen-mostert commented Jun 25, 2024

If you could add the to the git commit a comment line

Your wish is my command. Within reason. Which this is.

Another thing, did you also do a full rebuild to test that "eval" did not cause any problem?

Yes, as mentioned in my original post. I did a full rebuild from freshly checked out sources and ran the tests from the examples. After this I actually want to take shellcheck to town on build.sh because there are multiple places where the script issues the occasional warning during the build (like an [ test failing) -- they seem to be mostly benign but in a big build like this it's hard to tell, and extra quoting never hurts.

@jeroen-mostert
Copy link
Contributor Author

jeroen-mostert commented Jun 25, 2024

Well, I got the issue number wrong and made a mess out of things, so now we have TWO commit messages. I dare not mess with it further since this is git. :P If it's an issue I'll just redo the entire pull, I guess.

@jeroen-mostert
Copy link
Contributor Author

jeroen-mostert commented Jun 25, 2024

Never mind, I found the magic invocation that allows you to fix things. If it's not one form of --force it's another. Well, a day without learning is a day wasted.

This is probably the most amount of characters ever spent on someone adding -O3 somewhere, but that's programming for you.

@lamikr lamikr merged commit ab9cf31 into lamikr:master Jun 26, 2024
@lamikr
Copy link
Owner

lamikr commented Jun 26, 2024

Thay say that devil is in the details :-) But tested with clean build that this works now and could not find any problem on eval either. THANKS!!!

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