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

RFC: Patch openblas to add 64_ suffix when using 64 bit ints #8734

Merged
merged 5 commits into from
Oct 27, 2014

Conversation

tkelman
Copy link
Contributor

@tkelman tkelman commented Oct 19, 2014

Not done yet, but I think I've got part 2 of #4923 (comment) mostly finished - when 1 & 3 are completed this should close #4923. I still need to add the install steps, license link, etc for objconv on OSX which should be fairly short, and the Julia macro in base to account for the renamed functions. See #8734 (comment) for instructions on how to properly test this

@tkelman
Copy link
Contributor Author

tkelman commented Oct 19, 2014

cc @xianyi and @wernsaar, this is an attempt at fixing OpenMathLib/OpenBLAS#436 - please let me know if you have feedback on the openblas patch part of this, I intend to submit the patch upstream once we've got everything working.

@tkelman
Copy link
Contributor Author

tkelman commented Oct 19, 2014

Fortran sure is fun. I need a probably-gfortran-specific flag to turn on the preprocessor, but then since my fake patching arpack via -D flags increases the length of the blas/lapack function names, some lines exceed the default maximum length for fixed-format files, causing a confusing syntax error and needing another probably-gfortran-specific flag to disable. Is this cheap method of patching arpack too messy, anyone have opinions or better ways to do it?

@tkelman
Copy link
Contributor Author

tkelman commented Oct 19, 2014

555bc3f is probably the ugliest possible way to accomplish the symbol renaming, but it was the first method that I could get to work. All other approaches I tried either resulted in ccall complaining that the first argument was not a valid constant expression, or gave undefined variable errors when initializing LinAlg. Can someone who knows what they're doing when it comes to metaprogramming please help come up with a better solution?

@tkelman
Copy link
Contributor Author

tkelman commented Oct 19, 2014

Oh, and the instructions for installing objconv manually are as follows:

cd deps
curl -O http://www.agner.org/optimize/objconv.zip
unzip -d objconv objconv.zip
cd objconv
unzip source.zip
clang++ -o objconv -O2 *.cpp

Once that's translated into Makefile rules so it happens automatically, this should be essentially functional on all platforms Julia targets, I think. Subject to improvement in the macro department however.

objconv cannot do in-place modification of files

dont overwrite static library with objcopy on freebsd either

fix linking of arpack and suitesparse with 64_ blas suffix

the patched xlahqr2 was using UPPERCASE function names, and the probes
during arpack configure were using the C compiler, sgemm_ and cheev_
need to enable c preprocessor, and disable syntax error on long lines

SPQR also needed -DSUN64
@tkelman
Copy link
Contributor Author

tkelman commented Oct 20, 2014

objcopy should be installed by default with binutils on FreeBSD, right? @nolta ?

@nolta
Copy link
Member

nolta commented Oct 20, 2014

Yep

@tkelman
Copy link
Contributor Author

tkelman commented Oct 20, 2014

Thanks, good to know that Mac is the only place where doing this has to introduce a new dependency that won't already be there.

@stevengj did you have any reasons for recommending a macro over a function for the purposes of appending the blas/lapack function name suffix? Every ccall in either base/linalg/blas.jl or base/linalg/lapack.jl is already inside an @eval, so it is actually simpler (for me anyway) to do the appending as a function rather than a macro. Compare e54144d vs 555bc3f - I think the former is just as easy to modify as the latter, but simpler in implementation.

@stevengj
Copy link
Member

If you just want something to append a suffix to a symbol, it should just be a function.

I was thinking about something to replace the ccall itself, i.e. @blascall(:dgemm, ....) would turn into ccall((:dgemm64, libblas), ...). This is only possible with a macro because ccall is magic.

But since the ccalls are already generated as you point out, it's probably just as easy to do it the way you are doing it.

@stevengj
Copy link
Member

I wouldn't call the function blas_suffix, I would just make it blasfunc. It may in the future perform other transformations (e.g. to link to some system BLAS libraries you might need to convert the name to all uppercase).

@vtjnash
Copy link
Member

vtjnash commented Oct 21, 2014

if you want to see an existing (working) attempt at running the preprecessor on fortran code, while changing symbol length, see the gfortblas rules:

GFORTBLAS_FFLAGS += -cpp -ffree-line-length-0 -ffixed-line-length-0 \

@tkelman
Copy link
Contributor Author

tkelman commented Oct 21, 2014

@vtjnash ah there you go. I got it working, I used -cpp -ffixed-line-length-none instead of -cpp -ffixed-line-length-0 but pretty similar

@tkelman tkelman changed the title WIP: Patch openblas to add 64_ suffix when using 64 bit ints RFC: Patch openblas to add 64_ suffix when using 64 bit ints Oct 21, 2014
@tkelman
Copy link
Contributor Author

tkelman commented Oct 21, 2014

I think this is pretty much ready. Testing appreciated - you won't get the renamed symbols unless you do

make cleanall
make -C deps distclean-arpack distclean-suitesparse
cd deps/openblas-v0.2.12
patch -p1 < ../openblas-symbol-rename.patch
rm libopenblas.*

(and do the same in reverse, with patch -R, before you go back to master)

The objconv source link isn't versioned, we could choose to mirror and checksum a versioned copy of it if we want.

@tkelman
Copy link
Contributor Author

tkelman commented Oct 22, 2014

Any feedback and/or confirmation whether this works on systems I don't have access to? @ViralBShah? @andreasnoack?

@stevengj
Copy link
Member

Just tried it on MacOS 10.9, and it seems to work (tests pass).

@tkelman
Copy link
Contributor Author

tkelman commented Oct 22, 2014

Thank you @stevengj, that's good to hear. I did test the objconv install rules locally by tweaking the patch to use objconv on Linux, which can be used to do the same job as objcopy but isn't needed.

Base.blas_vendor() should return :openblas64 if the patch was applied all the way.

@xianyi
Copy link

xianyi commented Oct 23, 2014

@tkelman , by default, I prefer OpenBLAS generating LP64 interface. I like to add BOTH_LP64_ILP64 flag in Makefile.rule. Then, julia users can enable this flag to build LP64 and ILP64 at single library.

@tkelman
Copy link
Contributor Author

tkelman commented Oct 23, 2014

Thank you for the input @xianyi. I don't believe my patch changes the default in any way? But having both LP64 and ILP64 in the same binary, with ILP64 using modified (prefixed or suffixed) names, would be ideal, yes. Do you think that's possible to do without needing to compile the entire library twice?

@tkelman
Copy link
Contributor Author

tkelman commented Oct 23, 2014

Thinking out loud here, but what if we created a lightweight wrapper (as others have done for BLAS and LAPACK many times) designed to link to a renamed-symbols ILP64 library, that exports a conventional LP64 interface under the standard names, and just casts all int32 inputs to int64 to call the same compiled ILP64 implementation? This might introduce a little bit of performance penalty due to casting and indirection, but hopefully quite minor. It would likely save a lot of compilation time and binary size.

edit: hm that simple approach may work for blas, but probably not lapack since pivot arguments are integer arrays, so the cost of copying and casting the entire contents of those arrays would be more significant.

@xianyi
Copy link

xianyi commented Oct 23, 2014

@tkelman , I don't think it's a big issue to compile the entire library twice. Without setting DYNAMIC_ARCH, I think it is quick to build OpenBLAS on modern multi-core CPUs.

@tkelman
Copy link
Contributor Author

tkelman commented Oct 23, 2014

Well, for Julia we compile with DYNAMIC_ARCH enabled by default, and across a variety of platforms, some of which aren't so fast at compiling things. Openblas can take an hour or two to build with DYNAMIC_ARCH enabled on my Windows machine, even with -j4. Doubling that would be unpleasant.

Though our compiled system image is non-portable by default, perhaps we should modify our build options a little, even across dependencies, to have more consistent "quicker to build" mode vs "portable results" mode. Then it's a question of which do we make default, and fully documenting how to use the other option. Thoughts, @staticfloat @nalimilan etc?

@staticfloat
Copy link
Member

I remember; SCS.jl used to explicitly link to libopenblas.so, but then we realized we didn't need to do so, we can just leave the OpenBLAS functions undefined in the .so and leave it up to the dynamic linker to supply them to the SCS binary at link-time because they already exist within the Julia processes' namespace.

@staticfloat
Copy link
Member

So in a sense, with SCS.jl, we are taking advantage of the very problem we are faced with in the numpy case.

@tkelman
Copy link
Contributor Author

tkelman commented Sep 15, 2015

Yeah looks like in that case i'd just need to rebuild new windows binaries since the windows linker doesnt allow undefined symbols

@staticfloat
Copy link
Member

This gives me an idea; if we don't want to rename libopenblas.so, we can probably dynamically generate stub functions (in julia!) that are exported and callable by numpy, thinking that it's using a non-64bit interface. As amazingly cool as that would be, it's probably more trouble than it's worth.

@tkelman
Copy link
Contributor Author

tkelman commented Sep 15, 2015

For blas that wouldnt be bad but anything in lapack with integer pivot vectors would be more work

@nalimilan
Copy link
Member

FWIW, in Fedora, the ILP64 OpenBLAS is called libopenblas64.so (unfortunately, it doesn't add the 64 suffix to symbols). This sounds right to me, since the ABI is incompatible with the "standard" libopenblas.so.

@stevengj
Copy link
Member

@nalimilan, this means that we shouldn't use libopenblas64 either, because then we will conflict with Fedora's.

We should just use libopenblas_julia, to make sure there are no accidental name collisions with distro choices.

@tkelman
Copy link
Contributor Author

tkelman commented Sep 15, 2015

Out of curiousity is it marked as conflicting with the lp64 package? Or just user beware, better not try to use both in same app?

@nalimilan
Copy link
Member

No, it doesn't conflict. That would be even more impractical since you wouldn't be able to install it if you have at least one app using the LP64 version.

But I don't think the whole issue has really been thoroughly considered. Maybe we could get the symbols to be renamed if we could find some agreement with upstream about what's the right solution.

@stevengj
Copy link
Member

The whole point is that this is a private copy of the libopenblas library for use in Julia, whose ABI is unlikely to match that installed by a distro or expected by any other library we might link in. Why shouldn't we name it whatever we want?

Isn't this basically the same discussion that we went through when we decided to rename the symbols in the first place? Renaming the library is just completing the job.

@tkelman
Copy link
Contributor Author

tkelman commented Sep 15, 2015

Maybe we could get the symbols to be renamed if we could find some agreement with upstream about what's the right solution.

I'd recommend you try to do so before it accumulates too many dependents.

@tkelman
Copy link
Contributor Author

tkelman commented Sep 15, 2015

We can name it whatever we want but there are libraries (elemental.jl will also need to handle this) that are capable of using our custom abi version. So it should be a sane and predictable name and we should try not to change it any more often than we have to. Once should be fine.

@nalimilan
Copy link
Member

Yes, let's avoid repeating the previous discussion. :-)

I agree it's not an issue if it's private, but if the same convention was used in Fedora as in Julia, I would be able to use the ILP64 OpenBLAS in my RPM package without risking any conflicts, which would be nice. If all distributions chose the same convention, more projects could easily use ILP64. OTC, if two distributions use a different one, it will never happen, which could also hurt Julia in cases where you'd like calls to an external library to support large arrays.

I've filed an issue at #4923.

stevengj added a commit to stevengj/julia that referenced this pull request Oct 1, 2015
…we also need to rename the library to avoid conflicts
stevengj added a commit to stevengj/julia that referenced this pull request Oct 1, 2015
…we also need to rename the library to avoid conflicts
stevengj added a commit that referenced this pull request Oct 2, 2015
…eed to rename the library to avoid conflicts

(cherry picked from commit b0bc951)
ref #13407
skumagai pushed a commit to skumagai/julia that referenced this pull request Oct 9, 2015
…we also need to rename the library to avoid conflicts
@nalimilan
Copy link
Member

For the record, Fedora and EPEL now ship with an additional ILP64 OpenBLAS package built with the 64_ suffix, and Julia has just started using it.

@eschnett
Copy link
Contributor

Do they actually use a suffix of 64_ (and not _64)? For example, Julia's OpenBLAS library is called libopenblas64_.dylib on my system, which is quite a strange name to choose. (Putting the underscore before the 64 or omitting the underscore would be more natural.)

I understand the idea that the mangled 32-bit names already end with an underscore on most systems, so adding 64_ to the mangled name seems natural, although adding _64 to the un-mangled name might have made more sense (since it allows calling the 64-bit functions from Fortran, which might not be possible if the Fortran compiler uses a non-underscore naming convention). I also understand that that's a non-issue on Linux, or with gcc, or for Julia.

I want to create a build recipe for a 64-bit OpenBlas for Spack https://github.com/LLNL/spack, and I'd like to stick with the "standard", hence my question: What underscore name mangling and what library names do Fedore and EPEL use? Is the library called libopenblas64_.so, and does nm list a function called _dgemm_64_?

@tkelman
Copy link
Contributor Author

tkelman commented Dec 25, 2015

See #4923 (comment). Adding a uniform suffix at the end of all symbols was done for simplicity's sake. The combination of Julia and OpenBLAS appears to be the first widespread entirely-open-source example of using ILP64 BLAS and LAPACK, so we are pretty much making up our own convention here. Given how much more widespread gfortran is than any other modern Fortran compiler, I don't think we should worry much about other Fortran name mangling conventions until someone seriously wants to build Julia with such a compiler. (ifort on Windows will need this, but since that's likely to be used with MKL we aren't in control of the symbol names there.)

Renaming the BLAS/LAPACK symbols does make it slightly harder to switch between LP64 and ILP64 for a pure Fortran application where it might be as simple as -fdefault-integer-8 otherwise. I think imposing a bit more trouble on the increasingly rare case of pure Fortran applications is worth clearing up the name collisions and ensuing segfaults from trying to load separate LP64 and ILP64 library versions simultaneously.

@eschnett
Copy link
Contributor

@tkelman I completely agree that the symbols need to have a different name; everything else just leads to confusion and segfaults. It's also not really "slightly harder to switch" -- all that's missing are the respective interface declarations for Fortran...

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.

ABI conflicts due to 64-bit libopenblas.so