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

use SSE on x86 only, don't test for big-endian when not compiling included STK #73

Merged
merged 5 commits into from
Apr 10, 2016

Conversation

hzulla
Copy link
Contributor

@hzulla hzulla commented Feb 25, 2016

It is with pleasure that I can report that there is now a Debian package of sc3-plugins. However, it currently doesn't build on many platforms.

https://buildd.debian.org/status/package.php?p=supercollider-sc3-plugins

It appears that the SSE flags in the cmake configuration aren't set properly.

The SSE flags were used on all architectures except arm, however the flags should only be used on x86 CPUs.

Also, the big-endian test is only necessary if we don't use the system's STK.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@danstowell
Copy link
Member

fyi I'm NOT in a position to test this at all.

Reading the patch through: looks good to me. The "not set properly" was implemented and tested by me, on an arm (beaglebone) and a x64 (thinkpad), and definitely worked, so I'd be interested to hear what is wrong with it. Why does it fail, do you believe?

Your change looks like good practice, providing an explicit "include" list for the flag rather than excluding. Are you confident that "^(x86|amd64|i.86)" works, and catches all cases now and in the future? I ask this because if this check fails, the user will probably not notice. They will lose SSE speedups and simply think SC is inefficient...

@hzulla
Copy link
Contributor Author

hzulla commented Feb 25, 2016

Hi Dan,

your patch removed SSE flags on ARM, only. So yes, it works when tested on ARM and x86.

However, SSE is an x86-only flag and will break the compiler run if used on non-x86 hardware.

The Debian package is based on an earlier git commit prior to your patch. That's why the Debian package currently breaks for all non-x86 platforms, including ARM. With your patch, the Debian package would work on x86, ARM and nothing else.

I'm "fairly" confident that the regex matches all variants of the x86 architecture, I googled around and looked at the CMAKE sources and all I found were x86_64, amd64, i386, i586, i686.

There is a FindSSE cmake rule floating around in other open source projects, but I was unsure about its licensing status.

@fsateler
Copy link

Why not test for the flag directly?

Csound does this https://github.com/csound/csound/blob/develop/cmake/CompilerOptimizations.cmake#L24

@hzulla
Copy link
Contributor Author

hzulla commented Feb 25, 2016

Sounds like a more sensible solution. I'll test this. Will be right back.

@hzulla
Copy link
Contributor Author

hzulla commented Feb 25, 2016

Ok, that works.

It's not necessary to set the SSE flags twice, so I removed the x86 test from source/StkInst/CMakeLists.txt. I tested this and this will still build the STK and the STKInst UGen with sse turned on, as before.

However, before this the cmake rules would set the stackrealign flag for the included STK and the STKInst UGen, this flag is now removed.

@sonoro1234 - is the stackrealign flag needed? StkInst is the only plugin in this collection that sets this flag in its cmake rules. I see no mention of the flag in the stk build instructions

@danstowell
Copy link
Member

untested but LGTM

danstowell added a commit to supercollider/supercollider that referenced this pull request Feb 25, 2016
@hzulla
Copy link
Contributor Author

hzulla commented Feb 26, 2016

Patch helped fix the Debian build issue:

https://buildd.debian.org/status/package.php?p=supercollider-sc3-plugins

However I'm still unsure if the stackrealign flag was required or not.

@sonoro1234
Copy link
Contributor

@hzulla stackrealign is neccesary in scsynth for not sigsev with portaudio may be it is not in the plugins

@danstowell
Copy link
Member

@hzulla see supercollider/supercollider#1879 and tim's comment which gives a small improvement

@hzulla
Copy link
Contributor Author

hzulla commented Feb 26, 2016

@sonoro1234 thanks! stackrealign isn't set for the other plugins, it was only set in your cmake rules for StkInst.

Could you please checkout "my" patched version here and see if it's running as intended or will sigsev on your system with portaudio?

Also, could you please provide me with a simple StkInst UGen sample supercollider code snippet that I can use to test if it works properly?

@hzulla
Copy link
Contributor Author

hzulla commented Feb 26, 2016

@danstowell thanks, included.

@hzulla
Copy link
Contributor Author

hzulla commented Mar 2, 2016

Ok, so it works on my machine - but I only have Linux to test it with...

@crucialfelix crucialfelix merged commit 41a6d72 into supercollider:master Apr 10, 2016
@hzulla
Copy link
Contributor Author

hzulla commented Apr 19, 2016

Thanks.

@hzulla hzulla deleted the sse-on-x86-only branch April 19, 2016 07:55
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.

None yet

5 participants