Fix compile against lapack-3.10.0 - part 1 #21114

dwardor · 2021-11-24T19:48:07Z

TODO:

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

Pull Request Readiness Checklist

See details at https://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

dwardor · 2021-11-24T20:10:11Z

Fixes #21116

alalek · 2021-11-24T20:13:51Z

This change breaks Linux builds - LAPACK becomes disabled.

msva · 2021-12-02T13:05:10Z

This change breaks Linux builds - LAPACK becomes disabled.

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

devurandom · 2021-12-02T14:53:24Z

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 both sposv_ (e.g.) and LAPACK_sposv_ and setting a -D preprocessor definition and then in the code using:

#ifdef OPENCV_USE_OLD_LAPACK
#  define LAPACK_sposv_ sposv_
#endif

?

devurandom · 2021-12-02T14:54:34Z

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

msva · 2021-12-02T22:16:14Z

@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 · 2021-12-03T02:58:54Z

@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.

dwardor · 2021-12-04T21:15:32Z

Just pushed a new version that should be more generic per vpisarev's recommendations

defines the new configuration parameter OPENCV_USE_LAPACK_PREFIX set to OFF by default so as to change nothing
uses this configuration parameter to #define LAPACK_FUNC(f)

This still works on gentoo with lapack 3.10.0 by configuring OPENCV_USE_LAPACK_PREFIX=ON via ebuild
If someone can test on lapack < 3.9.1 and confirm it still works too...

Tried but failed to figure out how to test lapack version in OpenCVFindLAPACK.cmake (am a cmake noob).
If anyone has a hint to spare...

msva · 2021-12-05T00:27:02Z

CMakeLists.txt

@@ -527,6 +527,8 @@ OCV_OPTION(OPENCV_SEMIHOSTING         "Build the library for semihosting target
 OCV_OPTION(ENABLE_PYLINT              "Add target with Pylint checks"                            (BUILD_DOCS OR BUILD_EXAMPLES) IF (NOT CMAKE_CROSSCOMPILING AND NOT APPLE_FRAMEWORK) )
 OCV_OPTION(ENABLE_FLAKE8              "Add target with Python flake8 checker"                    (BUILD_DOCS OR BUILD_EXAMPLES) IF (NOT CMAKE_CROSSCOMPILING AND NOT APPLE_FRAMEWORK) )

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


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

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

dwardor · 2021-12-05T14:06:59Z

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

dwardor · 2021-12-05T19:25:22Z

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

Thank you for update!

Please take a look on comments below.

alalek · 2021-12-13T08:21:01Z

cmake/OpenCVFindLAPACK.cmake

@@ -156,8 +163,16 @@ if(WITH_LAPACK)
    if(WIN32 AND NOT OPENCV_LAPACK_SHARED_LIBS)
      set(BLA_STATIC 1)
    endif()
-    find_package(LAPACK)
+    find_package(LAPACK CONFIG)


CONFIG

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

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

alalek · 2021-12-13T08:24:43Z

cmake/OpenCVFindLAPACK.cmake

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


LAPACK_FUNC

Please use OPENCV_ / OCV_ prefix.

alalek · 2021-12-13T08:29:47Z

cmake/OpenCVFindLAPACK.cmake

@@ -51,6 +51,13 @@ macro(ocv_lapack_check)
    if(NOT "${OPENCV_CBLAS_H_PATH_${_lapack_impl}}" STREQUAL "${OPENCV_LAPACKE_H_PATH_${_lapack_impl}}")
      list(APPEND _lapack_content "#include \"${OPENCV_LAPACKE_H_PATH_${_lapack_impl}}\"")
    endif()
+    list(APPEND _lapack_content "
+#ifdef OPENCV_USE_LAPACK_PREFIX
+#define LAPACK_FUNC(f) LAPACK_##f


-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.

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

dwardor · 2021-12-20T10:07:45Z

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

dwardor · 2021-12-21T22:13:37Z

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...

Fix compilation against lapack >= 3.9.1 and 3.10.0 while not breaking older versions OpenCVFindLAPACK.cmake & CMakeLists.txt: determine OPENCV_USE_LAPACK_PREFIX from LAPACK_VERSION hal_internal.cpp : Only apply LAPACK_FUNC to functions whose number of inputs depends on LAPACK_FORTRAN_STR_LEN in lapack >= 3.9.1 lapack_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.1 If 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 Conditions Remove 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)

alalek

Well done!

@dwardor Thank you for the patch!

@msva @devurandom Thank you for the review proposals!

dwardor · 2021-12-22T14:31:32Z

Thanks for the various pointers on the way.

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

alalek · 2021-12-22T15:27:41Z

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

vpisarev self-assigned this Dec 3, 2021

dwardor mentioned this pull request Dec 4, 2021

Fix compilation against lapack-3.10.0 - Part 2 #21115

Closed

6 tasks

msva reviewed Dec 5, 2021

View reviewed changes

alalek reviewed Dec 13, 2021

View reviewed changes

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

alalek unassigned vpisarev Dec 22, 2021

dwardor added 2 commits December 22, 2021 09:37

Fix typos and grammar in comments

c8c309a

alalek approved these changes Dec 22, 2021

View reviewed changes

alalek merged commit 54c1800 into opencv:3.4 Dec 22, 2021

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.2 haikuports/haikuports#6854

Merged

alalek mentioned this pull request Dec 2, 2022

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

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compile against lapack-3.10.0 - part 1 #21114

Fix compile against lapack-3.10.0 - part 1 #21114

dwardor commented Nov 24, 2021 •

edited by alalek

dwardor commented Nov 24, 2021

alalek commented Nov 24, 2021

msva commented Dec 2, 2021

devurandom commented Dec 2, 2021

devurandom commented Dec 2, 2021

msva commented Dec 2, 2021

vpisarev commented Dec 3, 2021

dwardor commented Dec 4, 2021

msva Dec 5, 2021

dwardor Dec 5, 2021

dwardor commented Dec 5, 2021

dwardor commented Dec 5, 2021 •

edited

alalek left a comment

alalek Dec 13, 2021

dwardor Dec 19, 2021

alalek Dec 13, 2021

dwardor Dec 19, 2021

alalek Dec 13, 2021

dwardor Dec 19, 2021

dwardor commented Dec 20, 2021 •

edited

dwardor commented Dec 21, 2021

alalek left a comment

dwardor commented Dec 22, 2021

alalek commented Dec 22, 2021

Fix compile against lapack-3.10.0 - part 1 #21114

Fix compile against lapack-3.10.0 - part 1 #21114

Conversation

dwardor commented Nov 24, 2021 • edited by alalek

Pull Request Readiness Checklist

dwardor commented Nov 24, 2021

alalek commented Nov 24, 2021

msva commented Dec 2, 2021

devurandom commented Dec 2, 2021

devurandom commented Dec 2, 2021

msva commented Dec 2, 2021

vpisarev commented Dec 3, 2021

dwardor commented Dec 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwardor commented Dec 5, 2021

dwardor commented Dec 5, 2021 • edited

alalek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwardor commented Dec 20, 2021 • edited

dwardor commented Dec 21, 2021

alalek left a comment

Choose a reason for hiding this comment

dwardor commented Dec 22, 2021

alalek commented Dec 22, 2021

dwardor commented Nov 24, 2021 •

edited by alalek

dwardor commented Dec 5, 2021 •

edited

dwardor commented Dec 20, 2021 •

edited