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.

Sign up for GitHub

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

Jump to bottom

Precompile windows gcc 8 #1039

Merged
merged 3 commits into from Sep 17, 2021
Merged

Precompile windows gcc 8 #1039

merged 3 commits into from Sep 17, 2021

Conversation

rok-cesnovar
Copy link
Member

Summary:

This PR changes the makefile logic so that PRECOMPILED_HEADERS=true is automatically set on Windows if its using g++ 8 or newer (RTools 4.0 supplied toolchain).
Currently its not used on Windows by default.

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Rok Češnovar

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

rok-cesnovar
rok-cesnovar commented Sep 16, 2021
View reviewed changes
.github/workflows/main.yml
@@ -32,6 +32,7 @@ jobs:
shell: powershell
- name: Build Math libs & add to PATH
run: |
make print-PRECOMPILED_HEADERS
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a test that shows that its still disabled in the Github Action test with Rtools 3.5.

@stan-buildbot
Copy link
Contributor

Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 3.54 3.48 1.02 1.55% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 0.96 -3.72% slower
eight_schools/eight_schools.stan 0.09 0.1 0.95 -5.31% slower
gp_regr/gp_regr.stan 0.14 0.14 1.02 1.97% faster
irt_2pl/irt_2pl.stan 5.12 5.08 1.01 0.75% faster
performance.compilation 91.14 88.95 1.02 2.4% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 8.2 8.21 1.0 -0.07% slower
pkpd/one_comp_mm_elim_abs.stan 29.47 31.31 0.94 -6.24% slower
sir/sir.stan 130.11 119.62 1.09 8.06% faster
gp_regr/gen_gp_data.stan 0.03 0.03 0.99 -1.04% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan 3.05 2.98 1.02 2.3% faster
pkpd/sim_one_comp_mm_elim_abs.stan 0.39 0.37 1.05 5.09% faster
arK/arK.stan 2.02 2.02 1.0 -0.05% slower
arma/arma.stan 0.25 0.25 1.0 0.36% faster
garch/garch.stan 0.61 0.6 1.02 2.16% faster
Mean result: 1.00680435875

Jenkins Console Log
Blue Ocean
Commit hash: 602320b

Machine information ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

@rok-cesnovar rok-cesnovar mentioned this pull request Sep 16, 2021
Closed
23 tasks
SteveBronder
SteveBronder approved these changes Sep 16, 2021
View reviewed changes
Copy link
Contributor

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One quick Q but looks good!

makefile
@@ -108,7 +108,11 @@ endif
STAN_FLAGS=$(STAN_FLAG_THREADS)$(STAN_FLAG_MPI)$(STAN_FLAG_OPENCL)

ifeq ($(OS),Windows_NT)
ifeq ($(shell expr $(CXX_MAJOR) \>= 8), 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also be checking they are using gcc and not clang or would it be fine in that case as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clang on windows with cmdstan does not work at all (#736) so that didnt cross my mind. Will add a note there if we ever figure that out.

Other option would be to keep the precompiled headers off in the off chance someone tries this with clang on Windows?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the logic to also handle clang on Windows. As we have no idea if it works, I opted to disabling it by default.

@stan-buildbot
Copy link
Contributor

Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 3.45 3.5 0.99 -1.26% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 1.0 0.49% faster
eight_schools/eight_schools.stan 0.09 0.09 1.02 2.38% faster
gp_regr/gp_regr.stan 0.14 0.14 1.0 -0.14% slower
irt_2pl/irt_2pl.stan 5.07 5.05 1.01 0.57% faster
performance.compilation 91.5 88.85 1.03 2.89% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 8.37 8.19 1.02 2.06% faster
pkpd/one_comp_mm_elim_abs.stan 31.74 30.48 1.04 3.98% faster
sir/sir.stan 118.17 123.41 0.96 -4.44% slower
gp_regr/gen_gp_data.stan 0.03 0.03 0.99 -0.67% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan 3.03 3.01 1.0 0.44% faster
pkpd/sim_one_comp_mm_elim_abs.stan 0.37 0.37 1.0 -0.19% slower
arK/arK.stan 2.03 2.03 1.0 -0.0% slower
arma/arma.stan 0.25 0.25 0.99 -0.55% slower
garch/garch.stan 0.61 0.6 1.0 0.34% faster
Mean result: 1.0043108235

Jenkins Console Log
Blue Ocean
Commit hash: 8134766

Machine information ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

@rok-cesnovar rok-cesnovar merged commit bbed34f into stan-dev:develop Sep 17, 2021
@rok-cesnovar rok-cesnovar deleted the precompile_windows_gcc_8 branch September 17, 2021 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveBronder SteveBronder SteveBronder approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rok-cesnovar @stan-buildbot @SteveBronder