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

Fix LAPACK for f2c converted sources #4453

Closed

Conversation

AndreySokolovSC
Copy link
Contributor

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

AndreySokolovSC and others added 2 commits January 18, 2024 23:43
* Change logical type for 64bit interface
* Fix xerbla_ calls in several functons (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.
@martin-frbg
Copy link
Collaborator

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 ?

@AndreySokolovSC
Copy link
Contributor Author

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 ?

Hi Martin!

About Makefile.system changes:
Yes, commit 7e2c6eb 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 3eac96c (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 🤗.

@martin-frbg
Copy link
Collaborator

Sorry for the delayed response - the f2clapack script I used for converting is in #3539 (comment) - 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

@AndreySokolovSC
Copy link
Contributor Author

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.

@martin-frbg
Copy link
Collaborator

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.

@AndreySokolovSC
Copy link
Contributor Author

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.

@martin-frbg
Copy link
Collaborator

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)

@AndreySokolovSC
Copy link
Contributor Author

Fixed in #4601. This PR can be closed

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.

2 participants