lamikr / rocm_sdk_builder

Other
113 stars 8 forks source link

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

Closed jeroen-mostert closed 3 days ago

jeroen-mostert commented 4 days ago

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:

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 commented 3 days ago

If you could add the to the git commit a comment line, fixes https://github.com/lamikr/rocm_sdk_builder/issues/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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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!!!