-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Conversation
67e7f90
to
85180c0
Compare
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. |
Fortran sure is fun. I need a probably-gfortran-specific flag to turn on the preprocessor, but then since my fake patching arpack via |
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 |
Oh, and the instructions for installing
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
b99f16f
to
555bc3f
Compare
|
Yep |
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 |
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 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. |
I wouldn't call the function |
e54144d
to
2af8d20
Compare
2af8d20
to
02ff762
Compare
if you want to see an existing (working) attempt at running the preprecessor on fortran code, while changing symbol length, see the gfortblas rules: Line 949 in e3a74ee
|
@vtjnash ah there you go. I got it working, I used |
I think this is pretty much ready. Testing appreciated - you won't get the renamed symbols unless you do
(and do the same in reverse, with The objconv source link isn't versioned, we could choose to mirror and checksum a versioned copy of it if we want. |
Any feedback and/or confirmation whether this works on systems I don't have access to? @ViralBShah? @andreasnoack? |
Just tried it on MacOS 10.9, and it seems to work (tests pass). |
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.
|
@tkelman , by default, I prefer OpenBLAS generating LP64 interface. I like to add |
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? |
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. |
@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. |
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 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? |
I remember; |
So in a sense, with |
Yeah looks like in that case i'd just need to rebuild new windows binaries since the windows linker doesnt allow undefined symbols |
This gives me an idea; if we don't want to rename |
For blas that wouldnt be bad but anything in lapack with integer pivot vectors would be more work |
FWIW, in Fedora, the ILP64 OpenBLAS is called |
@nalimilan, this means that we shouldn't use We should just use |
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? |
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. |
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. |
I'd recommend you try to do so before it accumulates too many dependents. |
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. |
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. |
…we also need to rename the library to avoid conflicts
…we also need to rename the library to avoid conflicts
…we also need to rename the library to avoid conflicts
For the record, Fedora and EPEL now ship with an additional ILP64 OpenBLAS package built with the |
Do they actually use a suffix of I understand the idea that the mangled 32-bit names already end with an underscore on most systems, so adding 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 |
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. ( 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 |
@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... |
Not done yet, but I thinkI'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 forobjconv
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