Skip to content

Adding Intel LLVM support for rocRAND backend#218

Merged
mkrainiuk merged 10 commits intouxlfoundation:developfrom
TejaX-Alaghari:rocrand_hip_support
Nov 2, 2022
Merged

Adding Intel LLVM support for rocRAND backend#218
mkrainiuk merged 10 commits intouxlfoundation:developfrom
TejaX-Alaghari:rocrand_hip_support

Conversation

@TejaX-Alaghari
Copy link
Contributor

@TejaX-Alaghari TejaX-Alaghari commented Aug 1, 2022

Description

This PR is intended to extend the HIP backend of RNG domain, which is currently supported for hipSYCL compiler to Intel’s LLVM compiler.

  • Added the build config for rocRAND and API launch with

All Submissions

  • Have you formatted the code using clang-format?

New features

  • Have you provided motivation for adding a new feature?

test_log.txt

@mmeterel
Copy link
Contributor

@TejaX-Alaghari Thanks for the PR. Similar comments from #189 applies here as well. Can we first focus on #189 and then apply the similar updates to this PR as well?

Copy link
Contributor

@aelizaro aelizaro left a comment

Choose a reason for hiding this comment

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

@TejaX-Alaghari, thank you for the PR! I have only 1 major question about engine passed to lambda by reference and several minor question.

Also, I see that Gaussian distribution tests failed for USM, do you know the root cause?

@@ -0,0 +1,46 @@
#--===============================================================================
# Copyright 2020-2022 Intel Corporation
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Not sure about copyright

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a new file, the copyright should contain only the year 2022.

Copy link
Contributor

@aelizaro aelizaro 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, are all issues in tests addressed? (there were some failures in the attached log)

@TejaX-Alaghari
Copy link
Contributor Author

TejaX-Alaghari commented Oct 18, 2022

Looks good to me, are all issues in tests addressed? (there were some failures in the attached log)

The failures of these skip_ahead cases is a known issue in rocRAND and has been mentioned in this oneMKL PR (ref Note2).

@TejaX-Alaghari
Copy link
Contributor Author

@vmalia @mkrainiuk Could you please review this PR and share your feedback as well.

@@ -0,0 +1,46 @@
#--===============================================================================
# Copyright 2020-2022 Intel Corporation
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a new file, the copyright should contain only the year 2022.

)
target_include_directories(
${LIB_OBJ} PRIVATE ${PROJECT_SOURCE_DIR}/include ${PROJECT_SOURCE_DIR}/src
${CMAKE_BINARY_DIR}/bin ${MKL_INCLUDE})
Copy link
Contributor

Choose a reason for hiding this comment

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

There are many whitespace changes in CMake files including copyright and cmake code. Can you please share why we are making those changes?

Copy link
Contributor Author

@TejaX-Alaghari TejaX-Alaghari Oct 27, 2022

Choose a reason for hiding this comment

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

We've observed that the format of this CMake file was not proper when compared with the other CMake files.
Hence we've ran the cmake-format tool on this file.

If you have any suggestion or recommendation on handling CMake file formats for oneMKL, do share with us.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I think it is a great idea. @mkrainiuk do you think it would help us to investigate this tool and add it to Contribution guidelines just like clang-format?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is a good idea to improve CMake file format too.

@TejaX-Alaghari
Copy link
Contributor Author

@aelizaro and @vmalia , following the recent commit to cuRAND backend, I've added support for handling kernel launch with hipSYCL and LLVM's host_task separately.

Requesting for review and merging the PR.

@TejaX-Alaghari
Copy link
Contributor Author

TejaX-Alaghari commented Nov 1, 2022

@aelizaro and @vmalia, thanks for all the suggestions and reviews.
@mkrainiuk, requesting for review.

Copy link
Contributor

@mkrainiuk mkrainiuk 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 for the PR, changes look good to me.

@mkrainiuk mkrainiuk merged commit b8ec0cf into uxlfoundation:develop Nov 2, 2022
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.

6 participants

Comments