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

linalg: refactor blas/lapack modules #913

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

jalvesz
Copy link
Contributor

@jalvesz jalvesz commented Dec 31, 2024

Major refactoring of the blas/lapack modules into submodules for improved build speed and potentially better organization of procedures. This PR follows #908

  • The reference files and refactoring script are currently within the legacy folder which should be removed before merging.
  • Two folders have been added: blas and lapack to organize all required files within.
  • The splitting is managed by the legacy\refactor_blaslapack_subm.py script which contains 3 dictionaries: blas_groups, lapack_groups, lapack_subgroups. The first two list the procedures in groups following lapacks documentation with some extra arbitrary splitting in order to improve parallel build. Given that the use of submodules requires a header module to declare the procedures signature, putting all lapack procedures into a single header implies a stall in the build process while building this single header. The dictionary lapack_subgroups proposes to split into modules which define macro-groups for the submodules.

@jalvesz jalvesz marked this pull request as ready for review December 31, 2024 11:10
Copy link
Contributor

@perazz perazz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jalvesz, it is very very good already. I only have a few comments. To summarize:

  • implicit none(type, external) -> implicit none, to satisfy implicit none(type,external) may not support by some fortran compiler #897
  • please remove the legacy/ folder altogether: it would cause confusion to have some code duplication. (thanks for leaving it there for comparison)
  • please do not use stdlib_blas_constants_* in the interfaces, as that increases parsing complexity, but no constants are used there.
  • Something in the module/submodule structure can be slightly streamlined.

src/blas/stdlib_blas_level1.fypp Outdated Show resolved Hide resolved
src/blas/stdlib_blas_level2_ban.fypp Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for providing the generating script.
Please delete it as it is not necessary to have it inside stdlib once the refactoring is merged.

src/blas/stdlib_blas_level2_gen.fypp Outdated Show resolved Hide resolved
src/blas/stdlib_blas_level2_pac.fypp Outdated Show resolved Hide resolved
src/lapack/stdlib_lapack_solve_aux.fypp Outdated Show resolved Hide resolved
src/lapack/stdlib_lapack_solve_chol.fypp Outdated Show resolved Hide resolved
src/lapack/stdlib_lapack_solve_ldl.fypp Outdated Show resolved Hide resolved
src/lapack/stdlib_lapack_solve_ldl_comp3.fypp Outdated Show resolved Hide resolved
src/lapack/stdlib_lapack_solve_ldl_comp4.fypp Outdated Show resolved Hide resolved
@jalvesz
Copy link
Contributor Author

jalvesz commented Jan 10, 2025

Thanks for your reviews @perazz! I've tried to address your comments in the latest commits. Let me try to summarize:

  • I removed the use statements in the interfaces, this was a residual from the automatic copying.
  • I did merge 2 of the auxiliaries in the base modules.
  • I replaced several constants values with the already available constants such as one, two, ten. There might be other that could be replaced.
  • I have renamed all the submodules following the convention stdlib_<blas>/<lapack>_subm_*. After some discussions with @ivan-pi I think keeping the implementations and the interfaces give an opportunity for enabling providing fixed interface modules as kind of public "headers" and the implementation details could be potentially changed by "just" replacing the submodule. So instead of merging for instance the module and submodule "others" I kept both but propose to have a naming strategy to make the distinction. I used subm out of lack of creativity, if a different naming would be preferable I can still make that change quite fast as I kept the script and the legacy files locally.

Copy link
Contributor

@perazz perazz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jalvesz for the review. I think it is almost ready.
I noticed the vast change in naming convention, and imho:

  • _subm_ should not be used: it does not conform with the submodule names everywhere else in the library. Also, we should try to stay away from names that become too long
  • For the same reason, a submodule would better not be used if there's only one (the auxiliary case). I suggest to not sacrifice clarity and simplicity now for the sake of a potential and currently unplanned future expansion :)

@jalvesz
Copy link
Contributor Author

jalvesz commented Jan 13, 2025

I see your points @perazz, the _subm_ was an idea, I'm fully open to a more structured and agreed upon naming convention. I also agree that too long path names are an issue. For instance building with Ninja under windows has a limit of 129 characters for the files path, so one has to be careful with that.

That being said, I do think that module/submodule splitting by:

  • Modules should contain only interfaces (or private implementations)
  • submodules the implementation for public procedures

Should be the preferred structuring for stdlib. There was a discussion here #540 around this topic.

Some ideas borrowed from @ivan-pi <>_sm, <>_impl, <>@name

@perazz
Copy link
Contributor

perazz commented Jan 17, 2025

@jalvesz as you point out there is a prior specific discussion, I think it is a separate topic, that will find agreement for the whole library, which should not be part of the current PR which addresses the BLAS/LAPACK backends. So my suggestion would be to restore the names you proposed in the initial commits, and then you would propose the naming convention change as part of a separate PR where it can be properly addressed and discussed (separation of concerns).

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