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 Options in C and Fortran APIs #152

Merged
merged 14 commits into from
Feb 4, 2024

Conversation

neil-lindquist
Copy link
Contributor

@neil-lindquist neil-lindquist commented Dec 6, 2023

After looking at things, I felt like the best way to implement slate_Options was to make it just a pointer to the C++ options object and provide accessor functions to read/write the various options. The slate_Options_create and slate_Options_destroy functions match the create/destroy functions for slate_Matrix/slate_Pivots/etc.

The options are set as, e.g., slate_Options_set_Lookahead( slate_Options opts, int64_t value ) and get as, e.g., slate_Options_get_Lookahead( slate_Options opts, int64_t defval ). So, normal usage would be

 // Execute on GPU devices with lookahead of 2.
 slate_Options opts = slate_Options_create();
 slate_Options_set_Target( opts, slate_Target_Devices );
 slate_Options_set_Lookahead( opts, 2 );

 slate_multiply_c32( alpha, A, B, beta, C, opts );

 slate_Options_destroy( opts );

To help simplify things, passing NULL as the options gets converted into an empty options array. This helps simplify cases where the defaults are good enough, as in most of our examples:

 // Execute on Host with lookahead of 1.
 slate_multiply_c32( alpha, A, B, beta, C, NULL );

Alternative approaches:

  • An array of Union types (i.e., what we had been doing).
    • We'd need to do something else for the Fortran API, but it'd be preferable to provide the same interface to both C and Fortran
    • The manually managed length is more tedious and is a point for user error
  • A struct of all the types
    • We'd still need a create function to set the defaults correctly
    • We'd need to document all the default values. And, I'm not certain how we'd have a default value for the bool options since assigning any non-zero value to C's bool will always assign 1.

Other improvements

  • Added a version of get_option that takes the Option argument as a template parameter and returns the expected type, instead of relying on the caller to specify the correct type.
    • I needed that mapping to generate all the getters and setters for the C/Fortran API anyways, so I figured I'd get the most out of it.
    • I replaced the get_option calls in LU (all 3) and Choleksy to make sure it was working.
  • Moved c_api/util.hh to src instead of include/slate. It's just internal functions, so it shouldn't be included in our external API.
  • Added BaseMatrix::num_devices() to the C/Fortran APIs (I needed it to only use Target::Devices in the gemm example when there are GPUs present.)
  • The C++ BLAS example was hardcoded to double precision, in spite of the functions being templated. So, I fixed that
  • Added two C examples and one Fortran example to examples/c_api and examples/fortran, respectively. Additionally, these examples were added to the CI tests.

@neil-lindquist neil-lindquist force-pushed the c-fort-options branch 2 times, most recently from 915f7da to 03560fa Compare December 14, 2023 13:09
@neil-lindquist neil-lindquist marked this pull request as ready for review December 14, 2023 13:16
@mgates3 mgates3 changed the title Fix Options in C and Fortran APIs (depends on #148) Fix Options in C and Fortran APIs Dec 19, 2023
Copy link
Contributor

@cayrols cayrols left a comment

Choose a reason for hiding this comment

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

Some minor comments need to be addressed first. But most of it is great!

examples/run_tests.py Show resolved Hide resolved
include/slate/types.hh Outdated Show resolved Hide resolved
src/getrf.cc Show resolved Hide resolved
tools/c_api/generate_matrix.py Show resolved Hide resolved
examples/c_api/Makefile Show resolved Hide resolved
examples/c_api/util.h Outdated Show resolved Hide resolved
examples/c_api/util.h Outdated Show resolved Hide resolved
examples/c_api/util.h Outdated Show resolved Hide resolved
examples/c_api/util.h Show resolved Hide resolved
examples/fortran/ex05_blas.f90 Outdated Show resolved Hide resolved
Copy link
Collaborator

@mgates3 mgates3 left a comment

Choose a reason for hiding this comment

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

Some comments. More to come.

examples/c_api/Makefile Show resolved Hide resolved
examples/fortran/Makefile Show resolved Hide resolved
examples/fortran/Makefile Show resolved Hide resolved
examples/fortran/Makefile Outdated Show resolved Hide resolved
examples/Makefile Outdated Show resolved Hide resolved
examples/fortran/util.f90 Outdated Show resolved Hide resolved
examples/fortran/util.f90 Outdated Show resolved Hide resolved
examples/fortran/util.f90 Outdated Show resolved Hide resolved
examples/run_tests.py Show resolved Hide resolved
tools/c_api/generate_matrix.py Show resolved Hide resolved
Copy link
Collaborator

@mgates3 mgates3 left a comment

Choose a reason for hiding this comment

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

I've fixed or am working on most issues, but haven't pushed changes quite yet.

examples/fortran/util.f90 Outdated Show resolved Hide resolved
examples/fortran/util.f90 Outdated Show resolved Hide resolved
tools/c_api/generate_matrix.py Outdated Show resolved Hide resolved
tools/c_api/generate_util.py Show resolved Hide resolved
examples/c_api/util.h Outdated Show resolved Hide resolved
examples/Makefile Outdated Show resolved Hide resolved
@mgates3 mgates3 force-pushed the c-fort-options branch 2 times, most recently from 05d5688 to 32c6ae4 Compare February 3, 2024 05:08
@mgates3
Copy link
Collaborator

mgates3 commented Feb 3, 2024

I added my fixes, rebased and fixed the merge conflict.

@mgates3 mgates3 mentioned this pull request Feb 3, 2024
11 tasks
@neil-lindquist
Copy link
Contributor Author

@mgates3 Thanks for addressing Sebastien's comments. I apparently missed the notification.

@mgates3 mgates3 force-pushed the c-fort-options branch 2 times, most recently from 202b094 to a0777c0 Compare February 3, 2024 19:48
@mgates3 mgates3 dismissed cayrols’s stale review February 4, 2024 03:33

Issues have been addressed.

neil-lindquist and others added 12 commits February 3, 2024 22:37
This also allows for a slightly more robust get_options function that automatically knows the right return type
It isn't part of our external API, so it shouldn't be included in the installation
* The example README had outdated information wrt pkg-config
* ex05_blas hardcoded double instead of using scalar_type
…plex matrix. Update grid_size to match C code. Fix processing command line arguments.
@mgates3 mgates3 merged commit 5e6d7ba into icl-utk-edu:master Feb 4, 2024
8 checks passed
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.

3 participants