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.

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

Add XE MMA/copy atom #23

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

rolandschulz
Copy link
Collaborator

No description provided.

@rolandschulz
Copy link
Collaborator Author

@rolandschulz rolandschulz mentioned this pull request Apr 11, 2024
Copy link
Collaborator

@mehdi-goli mehdi-goli 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 Roland for the PR! I have added some nitpick comments

// int W = size<1>(traits.tensor) * sizeof(typename decltype(traits.tensor)::engine_type::value_type);
int W = size<1>(traits.tensor) * sizeof(typename TD::value_type); //TODO: inconsistent to give the size in elements but use vector for copy
auto [y, x] = src.data().coord_;
XE_2D_LOAD::copy(traits.tensor.data().get(), W, H, W, int2_{x, y}, &*dst.data());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
XE_2D_LOAD::copy(traits.tensor.data().get(), W, H, W, int2_{x, y}, &*dst.data());
XE_2D_LOAD::copy(traits.tensor.data().get(), W, H, W, int2_{static_cast<int>(x), static_cast<int>(y)}, &*dst.data());

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's the motivation for this suggestion? Why do you want to allow narrowing conversion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am getting compile time errors when integrating this code into the PVC pipeline without the conversion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What tensor do you pass to the copy? What error do you get?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we resolve this? Maybe by merging as-is and open an issue to discuss this. Otherwise we have the other PRs depending on this having to make changes later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hope you don't mind I merged it without resolving this. Needed to clean up the PR dependency issue. Please open a new PR with your suggested change if you still think it is needed.

int H = size<0>(traits.tensor);
int W = size<1>(traits.tensor) * sizeof(typename decltype(traits.tensor)::engine_type::value_type);
auto [y, x] = dst.data().coord_;
XE_2D_SAVE::copy(traits.tensor.data().get(), W, H, W, int2_{x, y}, &*src.data());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
XE_2D_SAVE::copy(traits.tensor.data().get(), W, H, W, int2_{x, y}, &*src.data());
XE_2D_SAVE::copy(traits.tensor.data().get(), W, H, W, int2_{static_cast<int>(x), static_cast<int>(y)}, &*src.data());

@rolandschulz rolandschulz mentioned this pull request Apr 17, 2024
@rolandschulz rolandschulz merged commit ddbba2f into codeplaysoftware:sycl-develop Apr 17, 2024
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