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

[openblas] update to 0.3.15 version and add dynamic arch feature #15238

Merged
merged 27 commits into from
Jul 26, 2021

Conversation

xandox
Copy link
Contributor

@xandox xandox commented Dec 21, 2020

Add dynamic arch feature to OpenBLAS ports

  • What does your PR fix? Fixes # Add new feature

  • Which triplets are supported/not supported? Have you updated the CI baseline? Same

  • Does your PR follow the maintainer guide? yes

@NancyLi1013 NancyLi1013 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Dec 22, 2020
ports/openblas/CONTROL Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

@xandox

Have you tested the feature dynamic-arch?

xandox and others added 2 commits December 22, 2020 11:39
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
@xandox xandox changed the title [openblas] dynamic arch feature [openblas] update to 0.3.13 version and add dynamic arch feature Dec 22, 2020
@xandox
Copy link
Contributor Author

xandox commented Dec 22, 2020

@NancyLi1013 before PR I build with dynamic-arch feature for x64-linux, today I try to build for arm64-android and version 0.3.10 would not build. I also update version.

@xandox
Copy link
Contributor Author

xandox commented Dec 22, 2020

Oh, how to debug failed tests?

@NancyLi1013
Copy link
Contributor

For the failure on osx, there is no error log generated. I have rerun it and let's see the new test result.

For the feature test, you can test x86-windows and x64-windows.

@NancyLi1013 NancyLi1013 added the category:port-update The issue is with a library, which is requesting update new revision label Dec 23, 2020
ports/openblas/CONTROL Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

The failures are like this:

/Users/vagrant/Data/buildtrees/openblas/src/7079f26fa1-32aa3694bc.clean/kernel/x86_64/srot_microk_haswell-2.c:41:14: error: always_inline function '_mm256_fmadd_ps' requires target feature 'fma', but would be inlined into function 'srot_kernel' that is compiled without support for 'fma'
        t0 = _mm256_fmadd_ps(c_256, x0, t0);
             ^
/Users/vagrant/Data/buildtrees/openblas/src/7079f26fa1-32aa3694bc.clean/kernel/x86_64/srot_microk_haswell-2.c:42:14: error: always_inline function '_mm256_fmadd_ps' requires target feature 'fma', but would be inlined into function 'srot_kernel' that is compiled without support for 'fma'
        t1 = _mm256_fmadd_ps(c_256, x1, t1);
             ^
/Users/vagrant/Data/buildtrees/openblas/src/7079f26fa1-32aa3694bc.clean/kernel/x86_64/srot_microk_haswell-2.c:43:14: error: always_inline function '_mm256_fmadd_ps' requires target feature 'fma', but would be inlined into function 'srot_kernel' that is compiled without support for 'fma'
        t2 = _mm256_fmadd_ps(c_256, x2, t2);

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013
Copy link
Contributor

The same error on x64-osx

@NancyLi1013
Copy link
Contributor

Related PR #14760

@xandox
Copy link
Contributor Author

xandox commented Mar 4, 2021

Problem here is: openblas detects that processor (on check host) does not have FMA instructions set. And as it does not have this instructions -mfma flag does not append to compilator arguments. But openblas still use intrinsics from FMA set if processor is HASWELL and that is why error is produced.
I created issue OpenMathLib/OpenBLAS#3128 for openblas.

@xandox xandox closed this Mar 4, 2021
@xandox
Copy link
Contributor Author

xandox commented May 3, 2021

@martin-frbg thank you!

@xandox xandox requested a review from NancyLi1013 May 3, 2021 11:57
@xandox
Copy link
Contributor Author

xandox commented May 5, 2021

up

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels May 7, 2021
@NancyLi1013
Copy link
Contributor

LGTM now, thanks for your patience and contribution @xandox. Also thanks everyone here.


if(VCPKG_TARGET_IS_OSX)
if("dynamic-arch" IN_LIST FEATURES)
set(VCPKG_LIBRARY_LINKAGE dynamic)
Copy link
Contributor

Choose a reason for hiding this comment

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

We generally should not change library linkage if a feature is enabled. If a feature does not support a linkage type, we should output message(FATAL_ERROR). Does the dynamic-arch feature work for other platforms? We should also output an error if this feature is enabled on platforms that are not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There actually a issue not with openblas directly but in ninja.
https://gitlab.kitware.com/cmake/cmake/-/issues/16731
rsp-file contains 7001 lines of other libraries.

Can you suggest some other ways to resolve it without message(FATAL_ERROR)?

Copy link
Contributor

Choose a reason for hiding this comment

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

vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY)

plus a message about why this is happening.

@NancyLi1013 NancyLi1013 removed the info:reviewed Pull Request changes follow basic guidelines label May 18, 2021
@NancyLi1013 NancyLi1013 requested a review from dan-shaw May 19, 2021 07:40
@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013
Copy link
Contributor

@xandox
Could you please resolve the conflicts in this PR?

@xandox
Copy link
Contributor Author

xandox commented Jul 20, 2021

@xandox
Could you please resolve the conflicts in this PR?

done, once again

@NancyLi1013
Copy link
Contributor

LGTM now, thanks for your PR. @xandox

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jul 21, 2021
Copy link
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

This LGTM once merge conflicts have been solved

NancyLi1013 added 2 commits July 22, 2021 23:12
…blas_dynamic_arch_feature

# Conflicts:
#	ports/openblas/vcpkg.json
#	versions/baseline.json
#	versions/o-/openblas.json
@vicroms
Copy link
Member

vicroms commented Jul 25, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vicroms vicroms merged commit ce91faf into microsoft:master Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants