Repository navigation
feat(hipsparse): add the generated Fortran binding in bindings/fortran/hipsparse - #12368
amontoison wants to merge 8 commits into
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
9672e68 to
beb8830
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #12368 +/- ##
========================================
Coverage 75.95% 75.95%
========================================
Files 2880 2880
Lines 479025 479035 +10
Branches 71958 71958
========================================
+ Hits 363828 363838 +10
Misses 90338 90338
Partials 24859 24859
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
ca14ed1 to
d4144ef
Compare
04681a7 to
f6e5f02
Compare
f6e5f02 to
4b1851b
Compare
| # package is unusual, and it is done deliberately: the alternative is making | ||
| # every Fortran consumer of these bindings write enable_language(C) for a reason | ||
| # that has nothing to do with their code. | ||
| if(@HIPSPARSE_FORTRAN_NEEDS_C_PACKAGE@) |
There was a problem hiding this comment.
From what I understand, HIPSPARSE_FORTRAN_NEEDS_C_PACKAGE is never set?
|
hipsparse.F90 cannot be rendered (50k+ lines?) Can we not split this up? |
| # layout, like hipBLAS and the FFT libraries. | ||
| set(_fortran_pkgdir "${CMAKE_INSTALL_LIBDIR}/cmake/hipsparse") | ||
|
|
||
| install(TARGETS hipsparse_fortran |
There was a problem hiding this comment.
You need to set COMPONENT, else it defaults to runtime.
|
changelog entry is missing, what about docs? |
| # only linked, and libhipsparse resolves from the prefix path when the in-tree | ||
| # target is absent. Inheriting the guard would silently drop the bindings from a | ||
| # build that can perfectly well produce and install them. | ||
| add_subdirectory(fortran) |
There was a problem hiding this comment.
the enable_language(fortran) runs before this, so amdflang preferance is skipped.
| # them: one switch rather than two neighbouring ones. ON by default, and | ||
| # guarded the same way as the bindings, so asking for them when the bindings | ||
| # were not built is a skip rather than an error. | ||
| option(BUILD_FORTRAN_CLIENTS "Build the hipSPARSE Fortran clients and binding tests" ON) |
There was a problem hiding this comment.
It builds gpu test executables regardless of HIPSPARSE_ENABLE_CLIENTS, so a pure library build would build all of this. It should follow the clients/testings options.
| foreach(_hipsparse_c IN LISTS _hipsparse_cfgs) | ||
| set_target_properties(roc::hipsparse_fortran PROPERTIES | ||
| IMPORTED_LOCATION_${_hipsparse_c} | ||
| "${PACKAGE_PREFIX_DIR}/lib/fortran/${_hipsparse_dir}/libhipsparse_fortran.a") |
There was a problem hiding this comment.
lib/fortran hardcoded here, so this fails on lib64
kliegeois
left a comment
There was a problem hiding this comment.
It looks good and it looks like Nico's comments have been addressed.
| "not '${FORTRAN_ARRAY_INTERFACES}'.") | ||
| endif() | ||
|
|
||
| add_library(hipsparse_fortran STATIC hipsparse.F90) |
There was a problem hiding this comment.
The bindings are on by default for CUDA backend, so any consumer that pulls in the module object fails to link. Shall we disable the bindings for CUDA?
There was a problem hiding this comment.
@ntrost57 I don't understand this comment. The Fortran bindings will call the C routines of hipsparse. It is agnostic of the backend used to compile hipsparse (ROCm or CUDA).
| call hipCheck(hipMalloc(d_C, size_mat * 4)) | ||
| call hipCheck(hipMemcpy(d_csr_row_ptr, c_loc(h_csr_row_ptr(1)), size_rp * 4, hipMemcpyHostToDevice)) | ||
| call hipCheck(hipMemcpy(d_csr_col_ind, c_loc(h_csr_col_ind(1)), size_nz * 4, hipMemcpyHostToDevice)) | ||
| call hipCheck(hipMemcpy(d_csr_val, c_loc(h_csr_val(1)), size_nz * 8, hipMemcpyHostToDevice)) |
There was a problem hiding this comment.
h_csr_val is real(c_float) but the malloc and memcpy use *8 which goes out of bounds.
| # The Fortran samples used to compile the hand-written module themselves; they | ||
| # now link roc::hipsparse_fortran, so they cannot be built when the bindings | ||
| # were not. Turning the option off here rather than failing keeps | ||
| # -DHIPSPARSE_ENABLE_CLIENT=ON working on a machine with no Fortran compiler. |
There was a problem hiding this comment.
enable_language(Fortran) fails first, so this is never reached.
1d04dce to
b084201
Compare
b084201 to
500c8b1
Compare
Add fortran/hipsparse.F90, the hipsparse module generated from the C headers by the rocm-fortran generator under RFC0019 (ROCm/TheRock#8122). It is committed next to the headers, is not edited by hand, and .gitattributes marks it linguist-generated.
6ef741d to
a0669f5
Compare
…rated one
Build fortran/hipsparse.F90 as roc::hipsparse_fortran from fortran/, a
CMake project of its own that the root adds. BUILD_FORTRAN_BINDINGS
(default ${UNIX}, OFF on the CUDA backend), BUILD_FORTRAN_CLIENTS
(default ON) and FORTRAN_ARRAY_INTERFACES (default assumed-shape)
control it, and HIPSPARSE_BUILD_FORTRAN_* override per library.
The .mod and the archive install per compiler under
<includedir>/fortran/<compiler> and <libdir>/fortran/<compiler>, beside
a hipsparse-fortran config package.
Remove library/src/hipsparse.f90 and hipsparse_enums.f90, never
installed, so use hipsparse_enums becomes use hipsparse. The samples
link roc::hipsparse_fortran. fortran/test/ adds 52 CTest tests labelled
gpu; they use hip and skip until ROCm/rocm-systems#11923 lands.
Refs: ROCm/TheRock#8122 (RFC0019)
a0669f5 to
ef197da
Compare
…sparse The hipSPARSE Fortran binding consumes hipSPARSE's public API and builds standalone against an installed hipSPARSE, so it moves out of the library into bindings/fortran/hipsparse, the tree every monorepo and TheRock use for the Fortran bindings (RFC0015). Nothing is wired into the library build any more: the library is back to its state on develop. Its hand-written Fortran module stays for now: its deprecation and removal, with the Fortran clients that use it, follow in separate pull requests. - bindings/fortran/hipsparse reads hipSPARSE's version from projects/hipsparse - the binding has its own .gitattributes and CHANGELOG.md - CODEOWNERS: /bindings/fortran/hipsparse/, same owners - repos-config.json: register bindings/fortran/hipsparse so CI detects changes
Regenerate bindings/fortran/hipsparse with rocm-fortran-generator 9c8b3e4, and commit its metadata in bindings/fortran/hipsparse/metadata/, where the generator reads it from a checkout, instead of the copy kept in the generator.
Install the test executables in bin/, and a CTestTestfile.cmake in bin/hipsparse-fortran/ with the same tests and properties as the build tree, so that ctest --test-dir runs them from an install, as TheRock's test jobs do for the libraries.
The binding is built against an installed hipSPARSE and is never added to the library's build, so drop what only served that: the per-library BUILD_FORTRAN_* overrides, the PARENT_SCOPE results, the ROCM_HAVE_FORTRAN shortcut, the gating on the library's client and test options, and the comments about the root. Find the Fortran compiler through the cmake/ROCmFortranCompiler.cmake shared by all the bindings, and require CMake 3.19. The binding targets the ROCm backend only, so BUILD_FORTRAN_BINDINGS no longer defaults to OFF on the CUDA backend. The tests skip under BUILD_ADDRESS_SANITIZER, as in the other bindings, rather than under the library's HIPSPARSE_ENABLE_ASAN, and HIPSPARSE_ENABLE_HOST no longer matters.
…arse-fortran The package files went to lib/cmake/hipsparse, the C library's directory, with a forwarder in lib/cmake/hipsparse-fortran. rocRAND and hipRAND install their own legacy <lib>-fortran-config.cmake there, so the files overlapped. Install the config, targets and version files directly where find_package(<package>) looks, as the HIP and ROCTx bindings do, and drop the forwarder.
…ithout recursion rocm-cmake's generated hipsparse-config.cmake is to include lib/cmake/hipsparse-fortran/hipsparse-fortran-config.cmake after its targets (ROCm/rocm-cmake#303). Skip find_dependency(hipsparse) when hipsparse is the package being found, not only when roc::hipsparse is already imported, so that include never re-enters hipsparse-config.cmake.
Implements RFC0015, Fortran Bindings Split (update) for hipSPARSE:
bindings/fortran/hipsparse/is a standalone CMake project that builds the generated Fortran binding against an installed hipSPARSE. The library is untouched: no option, noadd_subdirectory, no change toprojects/hipsparse/.What lands
bindings/fortran/hipsparse/hipsparse.F90(61 288 lines, generated with ROCm/rocm-fortran-generator 9c8b3e4 from the hipSPARSE headers, markedlinguist-generated): one modulehipsparsewith thebind(C)interfaces, the enum constants, thehipsparseCheckhelper and array overloads for the routines the generator supports.metadata/hipsparse.toml: the generator metadata it was generated with.test/: 136 CTest tests, 82.f03tests of the C interfaces and 54.f08tests of the array overloads (registered as<name>_f2008). They also install: the executables inbin/, and aCTestTestfile.cmakewith the same tests and properties inbin/hipsparse-fortran/, forctest --test-dir.alpha,beta, dot results) aretype(c_ptr), valuein thebind(C)interfaces, so one interface covers both pointer modes.CMakeLists.txt, thehipsparse-fortranconfig template,cmake/ROCmFortranCompiler.cmake(the same in every binding: ROCm'samdflangis preferred over the first compiler onPATHunlessCMAKE_Fortran_COMPILERorFCis set),cmake/toolchains/and the binding'sCHANGELOG.md..github/CODEOWNERS:/bindings/fortran/hipsparse/ @ROCm/rocm-fortran-maintainers @ROCm/sparses-reviewers..github/repos-config.json: abindings/fortranentry, so CI detects changes to the binding. Each binding PR appends its entry at the end of the list, so all but the first to merge need a trivial rebase.A consumer uses
find_package(hipsparse-fortran)and linksroc::hipsparse_fortran. The archive and.modfiles install per compiler underlib/fortran/<compiler>/andinclude/fortran/<compiler>/;HIPSPARSE_FORTRAN_COMPILER_DIRnames that directory. The archive has one section per procedure, so a program linked with-Wl,--gc-sectionskeeps only the wrappers it calls.Options
BUILD_FORTRAN_BINDINGS${UNIX}BUILD_FORTRAN_CLIENTSONtest/; off underBUILD_ADDRESS_SANITIZERFORTRAN_ARRAY_INTERFACESassumed-shapenone/assumed-shape/assumed-rankarray overloadsIn TheRock
ROCm/TheRock#9446 builds
bindings/fortran/hipsparseas thehipsparse-fortransubproject against the hipSPARSE it built, with-DBUILD_FORTRAN_BINDINGS=ON -DBUILD_FORTRAN_CLIENTS=${THEROCK_BUILD_TESTING}, ships it in thesparse-fortranartifact and runs the installed tests in itshipsparse-fortranGPU test job.Follow-ups
This binding replaces the hand-written
library/src/hipsparse.f90andhipsparse_enums.f90(never installed), which stay for now. Deprecating them, then removing them and moving the library's Fortran clients to this binding, come in separate pull requests.Build and test
From the repository root, with
$PREFIXthe install prefix:$HIP_FORTRAN_PREFIXholds the HIP Fortran binding (hip-fortran) built with the same compiler; without it the tests are skipped.Verification
Standalone against ROCm 10.1 with amdflang on one GPU: 136 tests, 136 pass.
JIRA ID: ROCM-31390