Skip to content

Conversation

@staticfloat
Copy link
Member

@staticfloat staticfloat commented Jan 20, 2017

This fixes the compiler regression noticed in GCC 6.3.0 in #20123 by disabling -ftree-slp-vectorize as a side-effect of moving from -O3 to -O2. We prefer to compile with -O2 by default anyway, so let's do so.

@staticfloat staticfloat changed the title Add patch to set -O2 for suitesparse build [release-0.5] Add patch to set -O2 for suitesparse build Jan 20, 2017
@tkelman
Copy link
Contributor

tkelman commented Feb 11, 2017

as discussed in #20165, more of our makefiles default to -O3 than -O2, so would rather this be targeted to the architectures where it causes a problem, or be demonstrated that it actually doesn't make a difference in a test that thoroughly exercises suitesparse

@ViralBShah
Copy link
Member

It wasn't right for our Makefiles to use -O3 in the first place. The policy should be to use -O2 and -O3 in a targeted way, which is a well established practice. I am ok with merging this right away.

@tkelman
Copy link
Contributor

tkelman commented Feb 12, 2017

Not without a targeted test of the performance impact. It's not acceptable to slow down sparse linear algebra for everyone to avoid a compiler bug on a rarely used architecture.

@staticfloat
Copy link
Member Author

The distro packaging wisdom of Debian, Ubuntu, and Arch all agree that O2 should be the default. However, we are much more performance-oriented than these packages. So I think we should move over to O2 being the default unless there is a significant performance impact. I will measure the performance impact across a few different platforms, if you can come up with a good benchmark, Viral. I'm not sure we have a good suitesparse benchmark that we can use to measure speed, or if we do which parts are the most important.

@tkelman
Copy link
Contributor

tkelman commented Feb 12, 2017

I'm going to close this so we can discuss in a single place, the master PR #20165, until we've shown one way or another whether -O3 is making a difference.

@tkelman tkelman closed this Feb 12, 2017
@tkelman tkelman deleted the sf/suitesparse_O2_release0.5 branch March 19, 2017 13:08
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.

4 participants