Adding rocSOLVER support for LAPACK domain with hip backend #208

srilekhainkulu99 · 2022-06-16T06:00:07Z

Adding rocSOLVER support for LAPACK

Description

Raising PR with initial code base to provide rocSOLVER support for LAPACK domain with the HIP backend

Checklist

All Submissions

Do all unit tests pass locally? test_all_log.txt
Have you formatted the code using clang-format?

srilekhainkulu99 · 2022-06-16T13:40:28Z

src/lapack/backends/rocsolver/rocsolver_lapack.cpp

+    });
+}
+
+// TBD: rocSolver doesn't need/support scratchpad_size


we are not sure what to return as scratchpad size because rocsolver doesn't use scratchpad., Please suggest what should be return here.? or how it should be handled.?

I don't expect much trouble with returning 0. Users should be able to allocate as normal since SYCL 2020 spec now allows zero ranged buffers and usm allocations should just return a nullptr.

Thanks for the suggestion @ericlars.

I don't expect much trouble with returning 0. Users should be able to allocate as normal since SYCL 2020 spec now allows zero ranged buffers and usm allocations should just return a nullptr.

@ericlars , When zero is returned from "get_scratchpad_size" APIs, the following error has been reported during buffer creation in the test cases for HIP backend.
"SYCL buffer size is zero. To create a device accessor, SYCL buffer size must be greater than zero."

We've found that this issue was coming from creating accessor to an empty scratchpad buffer. We've addressed this in the latest commit

srilekhainkulu99 · 2022-11-01T04:38:19Z

@ericlars and @sknepper, I've updated the PR with the test case results. Can you please review and share your suggestions.

…er with HIP backend support

Added explicit sync through MACRO helpers

… buffers

sknepper

Thanks for the PR! I'm looking forward to having rocSOLVER support added for LAPACK. =)

I have not tried running your changes, just did a visual code review with multiple comments throughout. Thanks!

sknepper · 2022-11-04T16:55:14Z

README.md

@@ -167,12 +171,12 @@ Supported domains: BLAS, LAPACK, RNG
        </tr>
 	    <tr >
            <td align="center">AMD GPU</td>
-            <td align="center">AMD rocBLAS </td>


The table formatting looks off when viewing the README in your branch: https://github.com/srilekhainkulu99/oneMKL/tree/rocsolver_hip_support

Thanks for the PR! I'm looking forward to having rocSOLVER support added for LAPACK. =)

I have not tried running your changes, just did a visual code review with multiple comments throughout. Thanks!

Thank you for such a detailed feedback. I've addressed majority of the comments.
Do let us know if you have any further suggestions.

The table formatting still appears off, please address this. https://github.com/srilekhainkulu99/oneMKL/tree/rocsolver_hip_support

cmake/FindrocSOLVER.cmake

docs/building_the_project.rst

sknepper · 2022-11-04T17:09:39Z

include/oneapi/mkl/detail/backend_selector_predicates.hpp

+        static_cast<unsigned int>(queue.get_device().get_info<sycl::info::device::vendor_id>());
+    if (!(queue.get_device().is_gpu() && vendor_id == AMD_ID)) {
+        throw unsupported_device(
+            "", "backend_selector<backend::" + backend_map[backend::rocsolver] + ">",


Minor, but the spacing doesn't seem to match the spacing above it

It seems like line 135 with the queue get device is still misaligned compared to line 122 above.

Ran clang-format to fix the indentation of this section which differs from the format of the code with line #122

include/oneapi/mkl/lapack.hpp

src/lapack/backends/rocsolver/rocsolver_helper.hpp

src/lapack/backends/rocsolver/rocsolver_lapack.cpp

src/lapack/backends/rocsolver/rocsolver_scope_handle.hpp

jadurazo · 2022-11-04T22:46:24Z

include/oneapi/mkl/lapack/detail/rocsolver/lapack_ct.hpp

+#ifndef _DETAIL_ROCSOLVER_LAPACK_CT_HPP_
+#define _DETAIL_ROCSOLVER_LAPACK_CT_HPP_
+
+#include <CL/sycl.hpp>


Does this need to be guarded similarly as in include/oneapi/mkl/blas/detail/rocblas/blas_ct.hpp ?

#if __has_include(<sycl/sycl.hpp>) #include <sycl/sycl.hpp> #else #include <CL/sycl.hpp> #endif

jadurazo · 2022-11-04T22:48:02Z

include/oneapi/mkl/lapack/detail/rocsolver/onemkl_lapack_rocsolver.hpp

+
+#ifndef _ONEMKL_LAPACK_ROCSOLVER_HPP_
+#define _ONEMKL_LAPACK_ROCSOLVER_HPP_
+#include <CL/sycl.hpp>


Does this need to be guarded similarly as in include/oneapi/mkl/blas/detail/rocblas/onemkl_blas_rocblas.hpp ?

#if __has_include(<sycl/sycl.hpp>) #include <sycl/sycl.hpp> #else #include <CL/sycl.hpp> #endif

src/lapack/backends/rocsolver/rocsolver_scope_handle.cpp

* Adjusted years for copyrights of new files * Adjusted misalignments and formatting issues * Added default case for get_rocsolver_jobsvd * Removed the conditional check of (m < n) for gebrd * Removed cuda header from rocsolver_scope_handle.hpp * Added misssing __has_include checks for CL/SYCL.hpp inclusions * Removed circular dependency in include/oneapi/mkl/lapack/detail/rocsolver/lapack_ct.hpp * Removed the duplicated macro of ROCSOLVER_ERROR_FUNC_T * Added default case for get_rocsolver_jobsvd * Removed the conditional check of (m < n) for gebrd * Removed cuda header from rocsolver_scope_handle.hpp * Added misssing __has_include checks for CL/SYCL.hpp inclusions

…etrs scratchpad check in getrs_usm.cpp

README.md

…ck_rocsolver.hxx

…208)" This reverts commit f4866ab.

ericlars · 2022-11-30T20:40:11Z

Mistakenly merged before other approvers could take a look, if any issues please comment and we can revert.

srilekhainkulu99 changed the title ~~Adding rocSOLVER support for LAPACK domain through Intel DPCPP compil…~~ Adding rocSOLVER support for LAPACK domain with hip backend Jun 16, 2022

srilekhainkulu99 commented Jun 16, 2022

View reviewed changes

mmeterel assigned ericlars Jun 17, 2022

srilekhainkulu99 force-pushed the rocsolver_hip_support branch 2 times, most recently from 1855208 to eb5d3f8 Compare August 23, 2022 05:35

mmeterel mentioned this pull request Sep 21, 2022

Adding rocBLAS support for BLAS domain through Intel DPCPP compiler with HIP backend support #189

Merged

4 tasks

mmeterel assigned sknepper Sep 22, 2022

srilekhainkulu99 force-pushed the rocsolver_hip_support branch from db8ced4 to d60d0c6 Compare October 31, 2022 15:10

srilekhainkulu99 force-pushed the rocsolver_hip_support branch from ed598eb to 1822fbe Compare November 1, 2022 05:05

srilekhainkulu99 and others added 6 commits November 2, 2022 16:01

Adding rocSOLVER support for LAPACK domain through Intel DPCPP compil…

d1b768b

…er with HIP backend support

unwanted data removed

d063638

Adding synchronization mechanism for host_task and updates

93cd795

added dependencies 64

acb494b

Adjusted scratchpad sizes for rocSOLVER APIs

326ccbd

Added explicit sync through MACRO helpers

Updated the CMakeLists for rocSOLVER and added the doc fro rocSOLVER

ca2b2b7

srilekhainkulu99 force-pushed the rocsolver_hip_support branch from c206a5c to ca2b2b7 Compare November 2, 2022 23:07

Adjusted scratchpad size and removed accessor creation for empty SYCL…

285b3a1

… buffers

sknepper reviewed Nov 4, 2022

View reviewed changes

Adjusted the scratchpad sizes to the failing cases

1d35c39

jadurazo reviewed Nov 4, 2022

View reviewed changes

Teja Alaghari added 2 commits November 4, 2022 18:35

Adjusted the spacing in CMakeLists.txt and conditional for skipping g…

48b03d3

…etrs scratchpad check in getrs_usm.cpp

sknepper reviewed Nov 7, 2022

View reviewed changes

README.md Show resolved Hide resolved

Adjusted formats for README, building_the_project.rst and onemkl_lapa…

d8c4a72

…ck_rocsolver.hxx

srilekhainkulu99 force-pushed the rocsolver_hip_support branch from 46adfff to d8c4a72 Compare November 22, 2022 19:43

jadurazo approved these changes Nov 30, 2022

View reviewed changes

ericlars approved these changes Nov 30, 2022

View reviewed changes

ericlars merged commit f4866ab into uxlfoundation:develop Nov 30, 2022

ericlars added a commit that referenced this pull request Nov 30, 2022

Revert "Adding rocSOLVER support for LAPACK domain with hip backend (#…

2d59bc4

…208)" This reverts commit f4866ab.

Adding rocSOLVER support for LAPACK domain with hip backend #208

Adding rocSOLVER support for LAPACK domain with hip backend #208

Uh oh!

Conversation

srilekhainkulu99 commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

All Submissions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srilekhainkulu99 Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srilekhainkulu99 commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sknepper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TejaX-Alaghari Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericlars commented Nov 30, 2022

Uh oh!

Uh oh!

srilekhainkulu99 commented Jun 16, 2022 •

edited

Loading

srilekhainkulu99 Nov 1, 2022 •

edited

Loading

srilekhainkulu99 commented Nov 1, 2022 •

edited

Loading

TejaX-Alaghari Nov 22, 2022 •

edited

Loading