Skip to content

[SYCL][Matrix] Move joint_matrix's implementation to experimental::ma… #3584

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
Merged
merged 4 commits into from
Apr 22, 2021
Merged

[SYCL][Matrix] Move joint_matrix's implementation to experimental::ma… #3584

merged 4 commits into from
Apr 22, 2021

Conversation

yubingex007-a11y
Copy link
Contributor

…trix namespace

@yubingex007-a11y yubingex007-a11y requested a review from a team as a code owner April 21, 2021 03:44
@yubingex007-a11y yubingex007-a11y requested a review from bader April 21, 2021 05:09
@yubingex007-a11y yubingex007-a11y mentioned this pull request Apr 21, 2021
Merged
alexbatashev
alexbatashev previously approved these changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@alexbatashev alexbatashev left a comment

Choose a reason for hiding this comment

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

LGTM

alexbatashev
alexbatashev approved these changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@alexbatashev alexbatashev left a comment

Choose a reason for hiding this comment

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

Still LGTM

@dkhaldi dkhaldi requested a review from kbsmith-intel April 21, 2021 14:29
alexbatashev
alexbatashev suggested changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@alexbatashev alexbatashev left a comment

Choose a reason for hiding this comment

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

Put on hold to let Kevin and Dounia review.

kbsmith-intel
kbsmith-intel reviewed Apr 21, 2021
View reviewed changes
Copy link
Contributor

@kbsmith-intel kbsmith-intel left a comment

Choose a reason for hiding this comment

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

Why are there changes other than the specific namespace changes in this PR?

@dkhaldi
Copy link
Contributor

dkhaldi commented Apr 21, 2021

Why are there changes other than the specific namespace changes in this PR?

Are you referring to the folder name change?
I requested this change because the choice of the folders name was not good. For instance, we don't use the prefix intel for folder names.

Should we create two separate PRs for each of the changes?

@yubingex007-a11y
Copy link
Contributor Author

Why are there changes other than the specific namespace changes in this PR?

eh, @alexbatashev suggested that we use enable_if_t in his comments. I am ok if we change it back or keep code there. @kbsmith-intel

kbsmith-intel
kbsmith-intel reviewed Apr 21, 2021
View reviewed changes
sycl/include/CL/sycl/ONEAPI/matrix/matrix-amx.hpp Outdated
@@ -179,8 +180,8 @@ template <typename Group, typename T, size_t NumRows, size_t NumCols,
matrix_layout Layout>
struct joint_matrix<
Group, T, NumRows, NumCols, Layout,
typename std::enable_if<(NumRows <= tile_size) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these changes part of this PR? My understanding was this PR was for namespace and file-name changes specifically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, @alexbatashev suggested that we use enable_if_t in his comments. I am ok if we change it back or keep code there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that should be handled in a separate PR. While I agree that is an improvement, it is not part of fixing the namespace and file-name issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will move it into another new PR to handle it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with committing as a separate PR as well

kbsmith-intel
kbsmith-intel approved these changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@kbsmith-intel kbsmith-intel left a comment

Choose a reason for hiding this comment

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

LGTM

dkhaldi
dkhaldi approved these changes Apr 22, 2021
View reviewed changes
Copy link
Contributor

@dkhaldi dkhaldi left a comment

Choose a reason for hiding this comment

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

LGTM

@yubingex007-a11y
Copy link
Contributor Author

ping @bader

@bader bader merged commit c957c7b into intel:sycl Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexbatashev alexbatashev alexbatashev approved these changes

@dkhaldi dkhaldi dkhaldi approved these changes

@kbsmith-intel kbsmith-intel kbsmith-intel approved these changes

@LuoYuanke LuoYuanke Awaiting requested review from LuoYuanke

@againull againull Awaiting requested review from againull

@bader bader Awaiting requested review from bader

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yubingex007-a11y @dkhaldi @alexbatashev @kbsmith-intel @bader