NotificationsYou must be signed in to change notification settings
Fork56.4k
Star85.3k

Fix compile against lapack-3.10.0 - part 1#21114

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

alalek merged 2 commits intoopencv:3.4fromdwardor:patch-1

Dec 22, 2021

Merged

Fix compile against lapack-3.10.0 - part 1#21114

alalek merged 2 commits intoopencv:3.4fromdwardor:patch-1

Dec 22, 2021

Conversation

Copy link

Contributor

dwardor commentedNov 24, 2021•
edited by alalek
Loading

TODO:

alalek:~~update builders configuration (to resolve Mac builder report) if it is not caused byCONFIG usage in find_package~~ removedCONFIG
alalek: validate MKL integration

Pull Request Readiness Checklist

See details athttps://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

Copy link

ContributorAuthor

alalek commentedNov 24, 2021

This change breaks Linux builds - LAPACK becomes disabled.

Copy link

msva commentedDec 2, 2021

This change breaks Linux builds - LAPACK becomes disabled.

actually, it fixes linux builds. On systems with modern versions of lapack.

Copy link

devurandom commentedDec 2, 2021

This change breaks Linux builds - LAPACK becomes disabled.
actually, it fixes linux builds. On systems with modern versions of lapack.

I assume for this to be accepted it has to work with both old and modern versions of LAPACK?

Could this be done by checking for bothsposv_ (e.g.) andLAPACK_sposv_ and setting a-D preprocessor definition and then in the code using:

#ifdefOPENCV_USE_OLD_LAPACK#  defineLAPACK_sposv_ sposv_#endif

Copy link

devurandom commentedDec 2, 2021

This fixes#19846 (of which#21116 is a duplicate).

Copy link

msva commentedDec 2, 2021

@dwardor could you please add checks (to stay compatible with older lapack versions)?

And also merge another PR (the part2) here, as, actually, it seems to be related to the same exact issue, so no need to split it to another PR, IMO.

vpisarev self-assigned this

Dec 3, 2021

Copy link

Contributor

vpisarev commentedDec 3, 2021

@dwardor, thank you for the patch!

I'm sure, it's useful, but I think we need to do it in a proper way:

the version of Lapack needs to be detected somewhere in CMake or maybe using some#ifdef's inside the code.
all the calls to Lapack needs to be wrapped into a macro, e.g.

#ifdef OPENCV_USE_LAPACK_PREFIX#define LAPACK_FUNC(f) LAPACK_##f#else#define LAPACK_FUNC(f) f#endif...

then you should define or not OPENCV_USE_LAPACK_PREFIX, depending on the version.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

What do you think about setting it to "ON" based on detected lapack version (to ease downstream maintenance work)?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Sounds great to me! Just haven't figured out how to do it

Copy link

ContributorAuthor

dwardor commentedDec 5, 2021

This autodetects LAPACK_VERSION, at least on my gentoo box, and activates OPENCV_USE_LAPACK_PREFIX automatically... I've only used LAPACK_FUNC to the function that strictly need it...

My gut feeling is :

That (at least for lapack > 3.7.0) all lapack function calls should be using LAPACK_xxxx (whatever the lapack version) as those are the base functions names that are then converted by the preprocessor to xxxx_ or xxxx or XXXX_ or XXXX depending on platform/architecture via the macros in lapack.h and lapacke_mangling.h (and sometimes since lapack 3.9.1 the preprocessors also add an extra parameter depending on the platform/architecture/compiler. Right now opencv presumes the xxxx_ variant is the correct one for all and does not let the preprocessor do the work it is supposed to do (which causes the issues for the extra parameters with lapack >= 3.9.1)
Depending on lowest lapack version compiled agains, we could probably remove LAPACK_FUNC and OPENCV_USE_LAPACK_PREFIX totaly by converting all xxxx_ to LAPACK_xxxx in hal_internal.cpp

Copy link

ContributorAuthor

dwardor commentedDec 5, 2021•
edited
Loading

Been looking through all lapack-release versions... and c/c++ interfaces appeared back in 3.4.0 :
lapack.h's content was at the time inside lapacke.h
LAPACK_xxxx was still the default function name + preprocessor macros (not exactly the same) were also present...

#ifndef LAPACK_NAME#define LAPACK_NAME(lcname,UCNAME)  lcname##_#endif...#define LAPACK_sposv LAPACK_NAME(sposv,SPOSV)...void LAPACK_sposv( char* uplo, lapack_int* n, lapack_int* nrhs, float* a,                   lapack_int* lda, float* b, lapack_int* ldb,                   lapack_int *info );

So just using LAPACK_xxxx in opencv source and letting the preprocessor macros do their job is the universal solution for anything above lapack-3.4.0.

Am I right in saying no one is compiling opencv vs. anything <3.4.0 as it contains no c/c++ interface ?

LAPACK_NAME(lcname,UCNAME) was replaced by LAPACK_GLOBAL(lcname,UCNAME) in lapack-3.4.1

alalek reviewed

Dec 13, 2021

View reviewed changes

Copy link

Member

alalek left a comment

Choose a reason for hiding this comment

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

Thank you for update!

Please take a look on comments below.

cmake/OpenCVFindLAPACK.cmake Outdated

		set(BLA_STATIC 1)
		endif()
		find_package(LAPACK)
		find_package(LAPACK CONFIG)

Copy link

Member

alalekDec 13, 2021

Choose a reason for hiding this comment

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

CONFIG

Why do we need to block CMake's LAPACK module?

Copy link

ContributorAuthor

dwardorDec 19, 2021

Choose a reason for hiding this comment

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

I just use CONFIG so as to have access to LAPACK_VERSION. If I don't add CONFIG LAPACK_VERSION is undefined.

cmake/OpenCVFindLAPACK.cmake Outdated

		#ifdef OPENCV_USE_LAPACK_PREFIX
		#define LAPACK_FUNC(f) LAPACK_##f
		#else
		#define LAPACK_FUNC(f) f##_

Copy link

Member

alalekDec 13, 2021

Choose a reason for hiding this comment

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

LAPACK_FUNC

Please useOPENCV_ /OCV_ prefix.

Copy link

ContributorAuthor

dwardorDec 19, 2021

Choose a reason for hiding this comment

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

cmake/OpenCVFindLAPACK.cmake Outdated

		endif()
		list(APPEND _lapack_content"
		#ifdef OPENCV_USE_LAPACK_PREFIX
		#define LAPACK_FUNC(f) LAPACK_##f

Copy link

Member

alalekDec 13, 2021

Choose a reason for hiding this comment

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

-DOPENCV_USE_LAPACK_PREFIX=LAPACK_

It makes sense to define prefix in CMake.

Moreover, we don't need global compiler definition at all.
It is a generated file. So generate this content through CMake conditions.

Copy link

ContributorAuthor

dwardorDec 19, 2021

Choose a reason for hiding this comment

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

Ok will remove the global alltogether and generate based in CMake conditions

dwardor mentioned this pull request

Dec 19, 2021

LAPACK_xxx is the universal way to go for lapack >= 3.40 with c/c++ i…#21291

Closed

6 tasks

Copy link

ContributorAuthor

dwardor commentedDec 20, 2021•
edited
Loading

I'd prefer we use#21291 instead of this as per#21114 (comment)

Update :#21291 is dead due to imcompatibility with Accelerate LAPACK implementation so this pullrequest seems the way to move forward

Copy link

ContributorAuthor

dwardor commentedDec 21, 2021

Only issue seems to be Mac build (just a warning ?)...@alalek I don't know how to debug this as I don't have a Mac...

alalek unassignedvpisarev

Dec 22, 2021

dwardor added2 commits

December 22, 2021 09:37

Fix compile against lapack-3.10.0

623dc7b

Fix compilation against lapack >= 3.9.1 and 3.10.0 while not breaking older versionsOpenCVFindLAPACK.cmake & CMakeLists.txt: determine OPENCV_USE_LAPACK_PREFIX from LAPACK_VERSIONhal_internal.cpp : Only apply LAPACK_FUNC to functions whose number of inputs depends on LAPACK_FORTRAN_STR_LEN in lapack >= 3.9.1lapack_check.cpp : remove LAPACK_FUNC which is not OK as function are not used with input parameters (so lapack.h preprocessing of "LAPACK_xxxx(...)" is not applicable with lapack >= 3.9.1If not removed lapack_check fails so LAPACK is deactivated in build (not want we want)use OCV_ prefix and don't use Global, instead generate OCV_LAPACK_FUNC depending on CMake ConditionsRemove CONFIG from find_package(LAPACK) and use LAPACK_GLOBAL and LAPACK_NAME to figure out if using netlib's reference LAPACK implementation and how to #define OCV_LAPACK_FUNC(f)

Fix typos and grammar in comments

c8c309a

alalek approved these changes

Dec 22, 2021

View reviewed changes

Copy link

Member

alalek left a comment

Choose a reason for hiding this comment

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

Well done!

@dwardor Thank you for the patch!

@msva @devurandom Thank you for the review proposals!

alalek merged commit54c1800 intoopencv:3.4

Dec 22, 2021

Copy link

ContributorAuthor

dwardor commentedDec 22, 2021

Thanks for the various pointers on the way.

@alalek will this also make it into opencv:4.x ?

Copy link

Member

alalek commentedDec 22, 2021

Changes to 4.x will be propagated automatically during the next "Merge-3.4" PR:https://github.com/opencv/opencv/wiki/Branches

alalek mentioned this pull request

Dec 22, 2021

(4.x) Merge 3.4#21317

Merged

dwardor deleted the patch-1 branch

December 23, 2021 00:04

alalek mentioned this pull request

Dec 30, 2021

(5.x) Merge 4.x#21371

Merged

alalek mentioned this pull request

Feb 22, 2022

(5.x) Merge 4.x#21651

Merged

gillins mentioned this pull request

Apr 27, 2022

gdal: add recipe for 3.4.2haikuports/haikuports#6854

Merged

alalek mentioned this pull request

Dec 2, 2022

OpenCV doesn't compile with OpenBLAS= ON, Eigen=OFF#22906

Open

4 tasks

Labels

category: build/install category: core

		OCV_OPTION(ENABLE_PYLINT"Add target with Pylint checks" (BUILD_DOCSOR BUILD_EXAMPLES) IF (NOTCMAKE_CROSSCOMPILINGANDNOT APPLE_FRAMEWORK) )
		OCV_OPTION(ENABLE_FLAKE8"Add target with Python flake8 checker" (BUILD_DOCSOR BUILD_EXAMPLES) IF (NOTCMAKE_CROSSCOMPILINGANDNOT APPLE_FRAMEWORK) )

		OCV_OPTION(OPENCV_USE_LAPACK_PREFIX"Use LAPACK_ prefix for function calls with lapack >= 3.9.1."OFF)

Movatterモバイル変換

Uh oh!

Fix compile against lapack-3.10.0 - part 1#21114

Fix compile against lapack-3.10.0 - part 1#21114

Uh oh!

Conversation

dwardor commentedNov 24, 2021• edited by alalekLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

dwardor commentedNov 24, 2021

Uh oh!

alalek commentedNov 24, 2021

Uh oh!

msva commentedDec 2, 2021

Uh oh!

devurandom commentedDec 2, 2021

Uh oh!

devurandom commentedDec 2, 2021

Uh oh!

msva commentedDec 2, 2021

Uh oh!

vpisarev commentedDec 3, 2021

Uh oh!

dwardor commentedDec 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwardor commentedDec 5, 2021

Uh oh!

dwardor commentedDec 5, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwardor commentedDec 20, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

dwardor commentedDec 21, 2021

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

dwardor commentedDec 22, 2021

Uh oh!

alalek commentedDec 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dwardor commentedNov 24, 2021•
edited by alalek
Loading

dwardor commentedDec 5, 2021•
edited
Loading

dwardor commentedDec 20, 2021•
edited
Loading