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

add openmp on windows #115

Open
wants to merge 43 commits into
base: main
Choose a base branch
from
Open

add openmp on windows #115

wants to merge 43 commits into from

Conversation

xoviat
Copy link
Contributor

@xoviat xoviat commented Jan 31, 2021

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@xoviat
Copy link
Contributor Author

xoviat commented Jan 31, 2021

@conda-forge-admin rerender

recipe/meta.yaml Outdated Show resolved Hide resolved
xoviat and others added 2 commits January 31, 2021 14:29
Co-authored-by: h-vetinari <h.vetinari@gmx.com>
recipe/bld.bat Outdated Show resolved Hide resolved
@xoviat
Copy link
Contributor Author

xoviat commented Jan 31, 2021

@xoviat
Copy link
Contributor Author

xoviat commented Jan 31, 2021

I guess there's cache in conda-forge-admin that's waiting to catch up.

@h-vetinari
Copy link
Member

It can take up to 90min until the finished package has made it through to the content delivery network (CDN), from which it is downloaded. As soon as you see download numbers >1 for the newest build here, that should be the case.

@isuruf
Copy link
Member

isuruf commented Jan 31, 2021

We pin to flang 5 at the moment. We'll need to rebuild all the packages with flang 11. We have a bot that can do that. Just needs to find time to do that.

@xoviat
Copy link
Contributor Author

xoviat commented Jan 31, 2021

@isuruf where is that flang-5 defined? In some repository?

@h-vetinari
Copy link
Member

Here: https://github.com/conda-forge/conda-forge-pinning-feedstock/

Wasn't aware that flang was pinned as well - but didn't @xoviat say that the ABI is compatible? Would it make sense to just relax the pin, instead of having a migrator? Or would that be too risky? @isuruf

@xoviat
Copy link
Contributor Author

xoviat commented Jan 31, 2021

@h-vetinari flang ABI is NOT compatible! see conda-forge/flang-feedstock#9

@h-vetinari
Copy link
Member

Ah, sorry, my bad - brainfart.

@h-vetinari
Copy link
Member

Here's some docs about the kind of migration that would be necessary, as well as the currently open ones

@xoviat
Copy link
Contributor Author

xoviat commented Jan 31, 2021

come to think of it, when f18 eventually comes out in a year or two, we'll have the same situation where another patch to the packages will be required. Oh well.

@martin-frbg
Copy link

martin-frbg commented Mar 25, 2024

Not seen on Linux (though I had to remove all gcc-style instruction set options like -m64, -mavx to get it to compile with flang-18 at all). Error message (and Fortran code location) do not look like they're specifically related to OpenMP , more likely a general code generation issue in the Windows version of LLVM

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@h-vetinari
Copy link
Member

Good news @conda-forge/openblas: with the in-progress flang 19, we can build OpenBLAS and pass the test suite, both with/without OpenMP. 🥳

For more background: llvm/llvm-project#86463 & OpenMathLib/OpenBLAS#4768

There's a positively shocking performance difference in the test suite with/without OpenMP

pthreads build (i.e. without OpenMP)

100% tests passed, 0 tests failed out of 120

Total Test time (real) = 8389.82 sec

and the OpenMP-enabled one:

100% tests passed, 0 tests failed out of 120

Total Test time (real) =  93.20 sec

By way of comparison, the current pthread build on main (using the old, non-llvm flang) has:

100% tests passed, 0 tests failed out of 120

Total Test time (real) = 8150.37 sec

I honestly don't know why this difference is so large, but I'm assuming the pthreads variant has some issues (c.f. #160).

Furthermore, there are some issues with AVX512 instructions (see OpenMathLib/OpenBLAS#4789) which will have to be fixed in LLVM, but for now the workaround that OpenBLAS is using itself (NO_AVX512=1) works.

All in all, it looks like with flang 19 in ~September, we should finally be able to switch our Fortran compilers on windows over to llvm-flang.

CC @martin-frbg @conda-forge/core

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

@h-vetinari
Copy link
Member

@conda-forge/openblas, this is ready for review (aside from reverting 153ce91 and waiting for the release of LLVM 19, but that's just a question of ~2 weeks), PTAL! :)

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.

5 participants