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

[question] Should INTERFACE64 be ON or OFF by default? #1763

Closed
yurivict opened this issue Sep 15, 2018 · 29 comments

Comments

Projects
None yet
3 participants
@yurivict
Copy link
Contributor

commented Sep 15, 2018

Hi,

The FreeBSD port has this option, but it is "OFF" by default.
There is one project that expects INTERFACE64=ON.
Should INTERFACE64 be set to ON by default? Are there projects that require it to be OFF? If yes, what to do, how to accommodate them all?

Yuri

@brada4

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2018

It should be off by default to resemble reference BLAS, which is compiled with fortran default, always 32 bit integers.
32bit int for argument sizes is sufficient to handle 16GB input/output vectors(much more for matrices with 2 uint32 dimensions)
Should you wish to recompile all your client code with -i8 option of fortran you can have it from openblas or reference blas.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2018

If you look at freebsd math ports section, like hundreds of them depend on 32 bit int blas and lapack.

@martin-frbg

This comment has been minimized.

Copy link
Collaborator

commented Sep 15, 2018

Some projects and distributions already use SYMBOLPREFIX and/or SYMBOLSUFFIX in combination with INTERFACE64 to avoid conflicts when providing both versions of the library. See #646 and comments at end of Makefile.rule.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2018

@yurivict
FreeBSD port is not orchestrated from here. You should contact them to propose int64 build + naming.
I dont think they will take "one project require" as something to consider. At least you mention place with formal requirement down the road.

@yurivict

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2018

@martin-frbg I added -DSYMBOLSUFFIX:STRING=x -DINTERFACE64:BOOL=true and it didn't make any difference.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2018

It is plain arguments, not defines.
make $* SYMBOLSUFFIX=64_ INTERFACE64=1
(snooped from https://src.fedoraproject.org/cgit/rpms/openblas.git/tree/openblas.spec , not that it is standard of any kind, just that it is most visible implementation)
PS Note that there is a bug or specifics that
<nothing> is different from INTERFACE64=0, so there are 2 values only <nothing> and INTERFACE64=1 that have desired effects.

@yurivict

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2018

INTERFACE64 though works on the cmake level, and SYMBOLSUFFIX doesn't.
I use ninja, not gnu make.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2018

cmake interface is being improved all the time, still it misses some options that make takes.

I dont know if this option suffices for you?

Actually enabling 64bit interface will break whole lot of ports, probably set LIBNAMESUFFIX=64 in default Makefile to not degrade all dependent packages.

@yurivict

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2018

No, I am working on making the port more robust and being able to cover both situations.
These variables work only because the old version of the port installs binaries manually. The new version of the port will use cmake, and I can't figure out how to change the installed names through cmake.

@martin-frbg

This comment has been minimized.

Copy link
Collaborator

commented Sep 15, 2018

Seems SYMBOLSUFFIX is not properly passed on from system.cmake, but I think setting LIBNAMESUFFIX should work from cmake as well (which would at least provide the option to name the INTERFACE64 library libopenblas64.so or similar, though not to change to internal names like gemv_64_).

@yurivict

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2018

LIBNAMESUFFIX=64 doesn't seem to work on both cmake and make levels.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2018

Cmake files are fresh development, please stay with gmake as at present. As you see some essential options do not work at all.

@martin-frbg

This comment has been minimized.

Copy link
Collaborator

commented Sep 15, 2018

make INTERFACE64=1 LIBNAMESUFFIX=64 does work, producing (for instance) libopenblas_64_haswellp-r0.3.4.dev.a. With cmake, unfortunately neither SYMBOLSUFFIX nor LIBNAMESUFFIX appear to work, but at least INTERFACE64 is passed on correctly. Probably renaming the library post build is your best option right now if you need to use cmake.
(Trying to hack system.cmake to set OpenBLAS_LIBNAME instead of LIBNAME unfortunately leads to garbled names like liblibopenblas_64_haswellp-r0.3.4.dev.a.a - at first glance it does not look as if this ever worked.)

@brada4

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2018

For current freebsd port makefile one has to change interface64 libnamesuffix and umping up installer file versio, no cmake involved and nothig broken

@yurivict

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2018

It's not that simple, though, as to just add a suffix to the library.
include/openblas_config.h also changes. The project I am looking at uses include/openblas_config.h to determine if interface64 is enabled.

Ideally, you should allow to install them both at the same time and allow some projects link with one version, and other projects - with the other version.

Currently, cmake installs:

include/cblas.h
include/f77blas.h
include/lapacke.h
include/lapacke_config.h
include/lapacke_example_aux.h
include/lapacke_mangling.h
include/lapacke_utils.h
include/openblas_config.h
lib/libopenblas.so
lib/libopenblas.so.0
lib/libopenblas.so.0.3
libdata/pkgconfig/openblas.pc
share/cmake/OpenBLAS/OpenBLASConfig.cmake
share/cmake/OpenBLAS/OpenBLASConfigVersion.cmake
share/cmake/OpenBLAS/OpenBLASTargets-%%CMAKE_BUILD_TYPE%%.cmake
share/cmake/OpenBLAS/OpenBLASTargets.cmake

I suggest, you should organize them in this way:
The version with INTERFACE64=0 will install:

include/openblas/cblas.h
include/openblas/f77blas.h
include/openblas/lapacke.h
include/openblas/lapacke_config.h
include/openblas/lapacke_example_aux.h
include/openblas/lapacke_mangling.h
include/openblas/lapacke_utils.h
include/openblas/openblas_config.h
lib/libopenblas.so
lib/libopenblas.so.0
lib/libopenblas.so.0.3
libdata/pkgconfig/openblas.pc
share/cmake/OpenBLAS/OpenBLASConfig.cmake
share/cmake/OpenBLAS/OpenBLASConfigVersion.cmake
share/cmake/OpenBLAS/OpenBLASTargets-%%CMAKE_BUILD_TYPE%%.cmake
share/cmake/OpenBLAS/OpenBLASTargets.cmake

and the version with INTERFACE64=1 will install:

include/openblas64/cblas.h
include/openblas64/f77blas.h
include/openblas64/lapacke.h
include/openblas64/lapacke_config.h
include/openblas64/lapacke_example_aux.h
include/openblas64/lapacke_mangling.h
include/openblas64/lapacke_utils.h
include/openblas64/openblas_config.h
lib/libopenblas_64.so
lib/libopenblas_64.so.0
lib/libopenblas_64.so.0.3
libdata/pkgconfig/openblas64.pc
share/cmake/OpenBLAS/OpenBLASConfig64.cmake
share/cmake/OpenBLAS/OpenBLASConfigVersion64.cmake
share/cmake/OpenBLAS/OpenBLASTargets64-%%CMAKE_BUILD_TYPE%%.cmake
share/cmake/OpenBLAS/OpenBLASTargets64.cmake

This way there's going to be no intersection between the file sets, and two could be installed/used simultaneously. You install many headers, it's better to put them into a dedicated folder anyway.

If I just add another port flavor that would only install lib/libopenblas_64.so*, wrong include/openblas_config.h would be called, and this would be error-prone. Other projects will probably build in a wrong way.


IMO, you should just adjust the cmake build like this: add some suffixes. This way the FreeBSD port can be updated in a consistent way. Otherwise, I am reluctant to update the port in a way that is obviously wrong.

@yurivict

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2018

@martin-frbg

This comment has been minimized.

Copy link
Collaborator

commented Sep 16, 2018

Thanks for the PR. I assume the cmake project expects to be installed into a user-defined prefix (like /opt/OpenBLAS) which would normally take care of the "dedicated folder" topic. (Not sure how other distribution maintainers handle this though.) Similarly I think it might be better to use LIBNAMESUFFIX in keeping with what the pre-existing gmake setup uses, and not define it automatically for INTERFACE64 builds ? (Or should the gmake build be changed - in the end I think it would be nice if both used the same variables and default behaviour ?)

@yurivict

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2018

I assume the cmake project expects to be installed into a user-defined prefix (like /opt/OpenBLAS) which would normally take care of the "dedicated folder" topic.

No. The prefix that cmake uses is the same for all projects, usually /usr/local or /usr There can't be any exceptions, because otherwise there would be thousands of such prefixes. Both cmake and pkg-config can't find packages in custom prefixes, so this would defeat your own cmake and .pc files.

The way this patch does it would fit existing distros well.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2018

I think it is worth to plant different names for 64 bit api in current freebsd port's option via make waiting for cmake files to mature to level of make. Ubuntu uses lib and lib32 for multiarch, so xyz64 naming is not such a universally great assumption.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2018

i.e. it is plainly a packaging bug to have incompatible .so replacing one that many others rely on.
the fix is few lines , no openblas source patch required yet.
You have to communicate this to FreeBSD bugzilla, referencing this thread. It is highly likely that port maintainer will come forward with 0.3.3 upgrade along same update (e.g. to support *LAKE *ZEN* cpu lines). Also 8 processor threads by default are quite few when multiple 16-core NUMA nodes are welded in single ryzen or xeon cartridge....

@yurivict

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2018

No, port maintainer in this case is very dormant, he will likely not do such thing.
I essentially took over this matter for now, and am suggesting here the way that would work for all systems.

The current state of the port in unacceptable, because it doesn't properly cover INTERFACE64=1. Having it as an option isn't acceptable, because this option produces incompatible binaries. I am trying to make it a flavor that can be concurrently installed, hence I suggested this patch here.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2018

It is one line to add renaming of library files and another to change base of include64 files.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2018

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=231371
We wait for reaction or no reaction couple of weeks before drawing any premature conclusions - OK?

@yurivict

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2018

It is one line to add renaming of library files and another to change base of include64 files

This is what the attached PR here does.

We wait for reaction or no reaction couple of weeks before drawing any premature conclusions - OK?

This patch is approved and can be committed, but it only updates the port. It doesn't take care of the INTERFACE64=1 situation. I would like to fix INTERFACE64=1 together with the update because a lot of other ports depend on OpenBLAS, all of them need to be retested every time before committing.

@yurivict

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2018

This PR is going to be acceptable for all distros: Debian, Redhat, Arch, etc, because they all function this way. The patterns existing in other ports used in this patch. You can reach out to them and confirm if in doubt.

@brada4

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2018

your freebsd BZ item is untouched. You can attach PR there, since you yourself casted doubts that it will be compatible with 3 popular linux distributions after.

@yurivict

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2018

your freebsd BZ item is untouched.

It is pending resolution here, upstream. I could go ahead and commit flavoring of the port, but this would be a departure from the upstream version. This matter should be resolved here.

you yourself casted doubts that it will be compatible with 3 popular linux distributions after.

I didn't cast such doubts, quite the opposite.

@martin-frbg

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2018

Patch merged now (although I am not entirely happy with what seems to be a diverging implementation of the suffix handling).
Unfortunately the SYMBOLPREFIX/SYMBOLSUFFIX options I suggested earlier are not currently available from cmake as they are actually implemented by post-processing the library with a call to objcopy in exports/Makefile and I lack the time and experience to come up with an equivalent cmake implementation.

@martin-frbg

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2018

SYMBOLPREFIX and SYMBOLSUFFIX are now available and the explanation of the options has been improved a bit.

@martin-frbg martin-frbg closed this Oct 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.