Skip to content

Performance regression due to absence of lto #2008

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

Closed
wds15 opened this issue Aug 12, 2020 · 3 comments · Fixed by #2020
Closed

Performance regression due to absence of lto #2008

wds15 opened this issue Aug 12, 2020 · 3 comments · Fixed by #2020

Comments

@wds15
Copy link
Contributor

wds15 commented Aug 12, 2020

Description

The freshly merged static matrix refactor things do slow down stan programs significantly whenever lto is not turned on.

So as of now in the static matrix world we absolutely need the lto optimisation for good performance. Thus, our makefiles should ideally switch on lto whenever the compiler is capable enough of handling it. The user should not be required to add -flto to the make/local to get good performance

Example

See #2007 (comment)

where it is demonstrated that turning on lto gives back the old performance numbers.

Expected Output

Results should be produced as fast as in the pre static matrices.

Current Version:

v3.3.0

@SteveBronder
Copy link
Collaborator

So yes I'm working on this right now. I decided to toss the idea of using flto for clang-6 or 7 (it's very hard to tell which version you are on). So as long as the user is using gcc >= 5.0 and clang > 7 we should have everything plus a few extra wizzes and woos we can turn on

It's taking me a minute because a few compiler settings + flto can actually lead to a slow-down(!) / wrong values on the performance tests. My guess is that too many optimizations + flto is causing too much inlining and some other algebra optims can cause slightly different numerics

@SteveBronder
Copy link
Collaborator

@wds15 can you throw the model and data up into a gist? If not feel free to email it to me

@rok-cesnovar
Copy link
Member

This will be fixed with an upstream PR, so reopening this until that happens.

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 a pull request may close this issue.

3 participants