OpenBLAS
OpenBLAS copied to clipboard
OpenMathLib
Fix LAPACK for f2c converted sources
- Change logical type for 64bit interface
- Fix xerbla_ calls in several functions (Added 3rd argument with string size)
- Usage of generated macros in f2c files to determine length of string has been replaced by a common C function.
Thanks - I'm fairly certain the (somewhat unrelated) change to Makefile.system is already in the main develop branch.
Fixing the f2c-generated files must have been quite a lot of work, but it would probably make more sense to regenerate them with an improved script at some point. (Is the change from "int" to "integer" that seems to make up the majority of the changes really necessary ? I believe "int" is declared to be "integer" elsewhere in the header).
It would probably be better to limit changes to the risc-v branch to actual risc-v code, or alternatively to merge the current develop branch once, otherwise I fear we'll end up with a thousand conflicts when merging the risc-v branch some day ?
Thanks - I'm fairly certain the (somewhat unrelated) change to Makefile.system is already in the main
developbranch. Fixing the f2c-generated files must have been quite a lot of work, but it would probably make more sense to regenerate them with an improved script at some point. (Is the change from "int" to "integer" that seems to make up the majority of the changes really necessary ? I believe "int" is declared to be "integer" elsewhere in the header).It would probably be better to limit changes to the risc-v branch to actual risc-v code, or alternatively to merge the current develop branch once, otherwise I fear we'll end up with a thousand conflicts when merging the risc-v branch some day ?
Hi Martin!
About Makefile.system changes:
Yes, commit https://github.com/OpenMathLib/OpenBLAS/pull/4453/commits/7e2c6eb8b6c4d9fb6d1de071a4b14adb8db1134b was taken from develop branch. Without its impossible to build correct LAPACK with INTERFACE64=1 for RISC-V. Changes exactly repeat develop branch and there should be no problems with merge conflicts, I suppose.
About int to integer change:
In f2c converted sources integer type declared like typedef blasint integer;.
So INTERFACE64=0 integer is 32 bit, with INTERFACE64=1 is 64 bit.
Since Fortran sources compiling with -fdefault-integer-8 key with INTERFACE64=1, width of INTEGER and LOGICAL Fortran types is 64 bit.
Without these changes, the logical type in converted sources is always 32 bits, this leads to fails in LAPACK tests, which are written in Fortran and pass 64 bit LOGICAL values to LAPACK function with 32 bit logical type.
About regenerating sources with script: I don't have much information about the script that generated these files. Can you provide information regarding this script? It sounds reasonable if it is not recommended to change these converted files manually.
About merge conflits to develop branch:
I tried to merge changes with the develop branch, there is a conflicts only from xerbla_() call was fixed in develop branch https://github.com/OpenMathLib/OpenBLAS/commit/3eac96ccdc322fcc72a16cc989733f3a3359ce34 (missed it, my bad).
Llimit changes to the risc-v branch to actual risc-v code: If we do not want to merge changes that are not related to RISC-V I can open PR with these changes to develop branch. No problem. Hopefully the risc-v branch will be merged into the develop branch in the near future 🤗.
Sorry for the delayed response - the f2clapack script I used for converting is in https://github.com/OpenMathLib/OpenBLAS/pull/3539#issuecomment-1510773304 - it still has some drawbacks (like blindly adding helper functions that may not be needed by the converted file) and with over 2000 files in the LAPACK codebase it may not have been such a good idea to add the declarations to each instead of having them in a common header. Also the ancient f2c program still chokes on some Fortran90+ idioms that I had to hand-correct in a copy of the "official" LAPACK files back when I did the big conversion run. So it is probably going to be a lot of work either way - but re-synchronizing with the Fortran sources would also import a number of code fixes that went into those in recent months
Hi @martin-frbg, Thanks for details! Yes, it really is a lot of work and I don't have time for this now. In this case, if manually changing files is not recommended (even in develop), I will close this PR and create new issue describing problems until better times.
Yes, this should not be going on the risc-v branch anyway (which we hope to merge soon) and I believe I have identified a few more problems in the current set of converted files that must be the result of having used different revisions of the evolving script. For example the C/Z SLAQ sources appear to lack the header section for handling ILP64 completely. I have copied the change for "logical" to the script and will re-generate the C copies as soon as possible (probably during the weekend). But please do not delete your branch yet (btw I'm not sure I saw an instance of the "generated macros for string length have been replaced by C functions" in the diff ?) . Thanks for alerting me to the problem.
Yes, this should not be going on the risc-v branch anyway (which we hope to merge soon) and I believe I have identified a few more problems in the current set of converted files that must be the result of having used different revisions of the evolving script. For example the C/Z SLAQ sources appear to lack the header section for handling ILP64 completely. I have copied the change for "logical" to the script and will re-generate the C copies as soon as possible (probably during the weekend). But please do not delete your branch yet (btw I'm not sure I saw an instance of the "generated macros for string length have been replaced by C functions" in the diff ?) . Thanks for alerting me to the problem.
Great, Thank you very much!
About macros for string length: I found issues in /lapack-netlib/SRC/lsamen.c and lapack-netlib/SRC/xerbla_array.c files.
Macro for determining the length of a string is declared as #define i_len(s, n) (n), which seems strange to me.
Ah, I see it now in xerbla_array.c - this is a harmless oddity in f2c, the parser already determines the maximum length of the character variable and enters it as the second parameter for the i_len function (or macro), the actual length is never determined for some reason (probably portability - f2c is old and abandoned code)
Fixed in https://github.com/OpenMathLib/OpenBLAS/pull/4601. This PR can be closed