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

[SYCL][Joint Matrix][E2E] Add Joint Matrix tests for matrix dimension as function argument and runtime input #15429

Merged

Conversation

YixingZhang007
Copy link
Contributor

@YixingZhang007 YixingZhang007 commented Sep 18, 2024

Description:

  1. The test joint_matrix_bf16_fill_k_cache_arg_dim.cpp has been added in sycl/test-e2e/Matrix to validate passing matrix dimensions as function arguments, instead of templated parameters, to the joint_matmul function.
  2. The test joint_matrix_bf16_fill_k_cache_runtime_dim.cpp has been added in sycl/test-e2e/Matrix to validate that the program works for reading matrix dimensions at runtime instead of compile time.
  3. The joint_matrix_bf16_fill_k_cache_impl.hpp file has been modified to support the new tests.

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.

Added comments

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.

What about the runtime arguments test? I think we need that one as well.

@YixingZhang007
Copy link
Contributor Author

YixingZhang007 commented Sep 18, 2024

What about the runtime arguments test? I think we need that one as well.

Thanks for all the suggestions for this PR 👍

I am still working on this one. I’m trying to do it in a similar way as we have for joint_matrix_bf16_cache_arg_dim.cpp, where we reuse the code in joint_matrix_bf16_cache_impl.hpp. However, I found that there will be a lot of modifications needed in joint_matrix_bf16_cache_impl.hpp because we need to change the interface for the test() function. So, I’m not sure if I should create a separate *_impl.hpp for the joint_matrix_bf16_cache_runtime_dim test.

@dkhaldi
Copy link
Contributor

dkhaldi commented Sep 19, 2024

I am still working on this one. I’m trying to do it in a similar way as we have for joint_matrix_bf16_cache_arg_dim.cpp, where we reuse the code in joint_matrix_bf16_cache_impl.hpp. However, I found that there will be a lot of modifications needed in joint_matrix_bf16_cache_impl.hpp because we need to change the interface for the test() function. So, I’m not sure if I should create a separate *_impl.hpp for the joint_matrix_bf16_cache_runtime_dim test.

You may create a new joint_matrix_bf16_cache_runtime_dim_impl.hpp file that only contains the new test() function but uses same joint_matmul function from joint_matrix_bf16_cache_impl.hpp.

@YixingZhang007
Copy link
Contributor Author

I am still working on this one. I’m trying to do it in a similar way as we have for joint_matrix_bf16_cache_arg_dim.cpp, where we reuse the code in joint_matrix_bf16_cache_impl.hpp. However, I found that there will be a lot of modifications needed in joint_matrix_bf16_cache_impl.hpp because we need to change the interface for the test() function. So, I’m not sure if I should create a separate *_impl.hpp for the joint_matrix_bf16_cache_runtime_dim test.

You may create a new joint_matrix_bf16_cache_runtime_dim_impl.hpp file that only contains the new test() function but uses same joint_matmul function from joint_matrix_bf16_cache_impl.hpp.

Thanks for the suggestion! I have created a new joint_matrix_bf16_cache_runtime_dim_impl.hpp.

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

@YixingZhang007 YixingZhang007 marked this pull request as ready for review October 2, 2024 21:29
Copy link
Contributor

@YuriPlyakhin YuriPlyakhin left a comment

Choose a reason for hiding this comment

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

LGTM

@aelovikov-intel
Copy link
Contributor

@YixingZhang007 YixingZhang007 requested a review from aelovikov-intel 3 hours ago

I'm just back from vacation and this PR

  1. is specific to Matrix feature/tests
  2. has had lots of review comments throughout its history

@YixingZhang007 , can you please summarize what you expect from me specifically?

@YixingZhang007
Copy link
Contributor Author

@YixingZhang007 YixingZhang007 requested a review from aelovikov-intel 3 hours ago

I'm just back from vacation and this PR

  1. is specific to Matrix feature/tests
  2. has had lots of review comments throughout its history

@YixingZhang007 , can you please summarize what you expect from me specifically?

Hello Andrey, sorry for the confusion! I was trying to get assistance for merging the PR from @intel/llvm-gatekeepers, because I noticed I'm not authorized to merge the PR.

@aelovikov-intel aelovikov-intel merged commit d29060e into intel:sycl Oct 4, 2024
12 checks passed
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.

4 participants