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.

Sign up for GitHub

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

Jump to bottom

[SYCL][BLAS] Move portBLAS to UXL Foundation as generic SYCL BLAS #618

Merged
merged 17 commits into from
Dec 20, 2024
Merged

[SYCL][BLAS] Move portBLAS to UXL Foundation as generic SYCL BLAS #618

merged 17 commits into from
Dec 20, 2024

Conversation

s-Nick
Copy link
Contributor

@s-Nick s-Nick commented Dec 10, 2024 •
edited
Loading

Description

This PR aims to keep oneMath portBLAS backend compatible and usable after the upcoming changes to it. Everything will be moved to UXL Foundation as a generic SYCL BLAS implementation.
Here all occurences to portBLAS are removed and renamed to a more appropriate name.

Checklist

All Submissions

@s-Nick
Update CMake configuration to new name
c6794af 
First changes to CMake config to move from portBLAS to oneMath generic
blas kernels.

Signed-off-by: nscipione <[email protected]>
@s-Nick
Change namespace
ea4fa08 
Update namespace from portBLAS to generic.

Signed-off-by: nscipione <[email protected]>
@s-Nick
Rename and move files
c4d7e3e 
Moved all files portblas related to new `generic` name. Moved directory
path to `generic`.

Signed-off-by: nscipione <[email protected]>
@s-Nick
Update include files, namespace and function names
be6d149 
Update included headers to new name after removing portblas.
Update namespace from portblas to generic.
Functions that had portblas in their name have been renamed to use
`generic` instead.

Signed-off-by: nscipione <[email protected]>
@s-Nick
Update generic blas backend CMakeLists
4a907b9
@s-Nick
Update macros
4e835f9 
Remove PORTBLAS from macros in favor of GENERIC_BLAS
@s-Nick
Update documentations and comments
e6ce2ef 
Remove portBLAS from documentations and comments

Signed-off-by: nscipione <[email protected]>
@s-Nick
Fix after rebasing on oneMath renaming branch
a575044 
Fix renaming part from portBLAS to generic after rebased.

Signed-off-by: nscipione <[email protected]>
@s-Nick
Update README to reference new project name
cdd5e1e 
Signed-off-by: nscipione <[email protected]>
@s-Nick s-Nick requested a review from Rbiessy December 10, 2024 17:07
s-Nick
s-Nick commented Dec 10, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
@s-Nick
Fix formatting
8e59102 
Signed-off-by: nscipione <[email protected]>
Rbiessy
Rbiessy reviewed Dec 11, 2024
View reviewed changes
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
@s-Nick s-Nick marked this pull request as ready for review December 13, 2024 09:24
@s-Nick s-Nick requested review from a team as code owners December 13, 2024 09:25
andrewtbarker
andrewtbarker reviewed Dec 13, 2024
View reviewed changes
Copy link
Contributor

@andrewtbarker andrewtbarker left a comment

Choose a reason for hiding this comment

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

Generally looks good, I think the use of names in the documentation could be a bit more consistent.

One question (which probably should have been brought up earlier in this process, sorry): "generic BLAS" to me suggests any library that implements the BLAS standard (eg ESSL, AOCL, NVPL, BLIS, ...) but that is not what is meant here. So I guess the suggestion is to include "SYCL" a lot for clarity.

docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
@Rbiessy Rbiessy mentioned this pull request Dec 16, 2024
Closed
@s-Nick
Address PR comments
b88b452 
Signed-off-by: nscipione <[email protected]>
@s-Nick
Copy link
Contributor Author

s-Nick commented Dec 16, 2024

Thank you for your review @andrewtbarker
I understand what you mean, I'll give another look to documentation for name consistency. In the mean time, I addressed all your suggestions in b88b452

Rbiessy
Rbiessy approved these changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@Rbiessy Rbiessy left a comment

Choose a reason for hiding this comment

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

Thanks LGTM, if there are no more concerns I will merge this tomorrow morning!

Rbiessy
Rbiessy requested changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@Rbiessy Rbiessy left a comment

Choose a reason for hiding this comment

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

I noticed the GitHub CI is not running the portBLAS tests anymore as it still uses the portBLAS options. Can you rename the occurrences in https://github.com/uxlfoundation/oneMath/blob/develop/.github/workflows/pr.yml#L37?

CMakeLists.txt Show resolved Hide resolved
src/blas/backends/generic/CMakeLists.txt Show resolved Hide resolved
@s-Nick
Add CMake WARNING message in case old definitions are used
e8d22aa 
In case definition related to portBLAS are passed during configuration
emits a warning message, convert the flag to new one and continue with
proper configuration.

Signed-off-by: nscipione <[email protected]>
@s-Nick
Merge branch 'develop' of github.com:uxlfoundation/oneMath into pb_to…
2326c1e 
…_blas_kernel
@s-Nick
Update GitHub workflows
a97bcb2
@s-Nick
Copy link
Contributor Author

s-Nick commented Dec 18, 2024

@Rbiessy I updated the workflows in a97bcb2 and it is running the CI now

@zettai-reido
Copy link

Looks approachable.
Do we really need those pre-C++17 namespaces in cpp files instead of
onemath::blas::generic style?

@s-Nick
Copy link
Contributor Author

s-Nick commented Dec 19, 2024

Thank you for your feedback @zettai-reido.
There is not particular reason for that namespace a part from following the contributing guideline (NS4) https://github.com/uxlfoundation/oneMath/blob/develop/CONTRIBUTING.md#ns-namespaces
I think this change is outside the scope of this PR. If we want to update the style we would need to update the documentation and other backend. Could you open an issue for it?

andreyfe1
andreyfe1 approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@andreyfe1 andreyfe1 left a comment

Choose a reason for hiding this comment

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

Looks good to me overall. Thanks!

@Rbiessy Rbiessy merged commit db520e4 into uxlfoundation:develop Dec 20, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewtbarker andrewtbarker andrewtbarker approved these changes

@Rbiessy Rbiessy Rbiessy approved these changes

@andreyfe1 andreyfe1 andreyfe1 approved these changes

Assignees

@s-Nick s-Nick

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@s-Nick @zettai-reido @andreyfe1 @Rbiessy @andrewtbarker