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 : update support conditions #9394

Merged

Conversation

Alcpz
Copy link
Collaborator

@Alcpz Alcpz commented Sep 9, 2024


Updates SYCL supported OPs checks to be in line with the updates introduced in previous PRs, as test-backend-ops is currently failing due to unsupported datatypes in CONT and IM2COL.

Signed-off-by: Alberto Cabrera <alberto.cabrera@codeplay.com>
@github-actions github-actions bot added ggml changes relating to the ggml tensor library for machine learning SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language labels Sep 9, 2024
@Alcpz
Copy link
Collaborator Author

Alcpz commented Sep 9, 2024

@NeoZhangJianyu
Copy link
Collaborator

NeoZhangJianyu commented Sep 10, 2024

@Alcpz
UT is used to test the quality and capability.
We need to improve the code to make sure the UT pass rate is 100%, instead of skip the test cases.

Some UT cases are updated for CUDA improvement, so the new/updated cases are not passed with SYCL backend.
SYCL backend should be improved to support more OPs as CUDA, to keep up the llama.cpp capability to support more LLMs.

If skip such UT cases, we won't know what's the gap.

Fault UT cases feedback the issues.
We need to fix the issues as root cause.

In previous months, the UT pass rate is high (96%).
For example:
In commit-id (17e98d4),

IM2COL(type_input=f32,type_kernel=f16,dst_type=f32,ne_input=[10,10,3,1],ne_kernel=[3,3,3,1],s0=1,s1=1,p0=1,p1=1,d0=1,d1=1,is_2D=1): OK
  IM2COL(type_input=f32,type_kernel=f16,dst_type=f16,ne_input=[10,10,3,1],ne_kernel=[3,3,3,1],s0=1,s1=1,p0=1,p1=1,d0=1,d1=1,is_2D=1): OK

In latest commit: 2a358fb

IM2COL(type_input=f32,type_kernel=f32,dst_type=f32,ne_input=[10,10,3,1],ne_kernel=[3,3,3,1],s0=1,s1=1,p0=1,p1=1,d0=1,d1=1,is_2D=1): crash....

We can see the new test cases are added:

test_cases.emplace_back(new test_im2col(GGML_TYPE_F32, GGML_TYPE_F32, GGML_TYPE_F32));
test_cases.emplace_back(new test_im2col(GGML_TYPE_F32, GGML_TYPE_F32, GGML_TYPE_F32, {3000, 128, 1, 1}, {3, 128, 1280, 1}, 1, 0, 1, 0, 1, 0, false));.

There is no reason SYCL can't support them.
We need to fix the code to support the test cases.

It's same for other fault cases.

So, I suggest to keep the original supports_op() function.

@Alcpz
Copy link
Collaborator Author

Alcpz commented Sep 10, 2024

@NeoZhangJianyu Thanks for the quick review.
I understand your point, but our internal CI is crashing and hanging because the SYCL backend doesn't currently support these operations, so I must disagree. Our priority is to ensure everything passes, and if we don't fix this now, we risk introducing breaking changes without realizing it. The support_op should reflect this, which is the purpose of this PR.

That said, I don't want to imply that we don't plan to support this in the future, so I will add a TODO to address it at a later time.

Copy link
Contributor

@OuadiElfarouki OuadiElfarouki left a comment

Choose a reason for hiding this comment

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

LGTM!

@NeoZhangJianyu
Copy link
Collaborator

@NeoZhangJianyu Thanks for the quick review. I understand your point, but our internal CI is crashing and hanging because the SYCL backend doesn't currently support these operations, so I must disagree. Our priority is to ensure everything passes, and if we don't fix this now, we risk introducing breaking changes without realizing it. The support_op should reflect this, which is the purpose of this PR.

That said, I don't want to imply that we don't plan to support this in the future, so I will add a TODO to address it at a later time.

@Alcpz
OK, I agree.

Thank you!

@NeoZhangJianyu NeoZhangJianyu merged commit 51b6038 into ggerganov:master Sep 11, 2024
52 checks passed
dsx1986 pushed a commit to dsx1986/llama.cpp that referenced this pull request Oct 29, 2024
* sycl : update support condition to im2col

Signed-off-by: Alberto Cabrera <alberto.cabrera@codeplay.com>

* Added TODO to remind supporting FP32 im2col

---------

Signed-off-by: Alberto Cabrera <alberto.cabrera@codeplay.com>
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Nov 15, 2024
* sycl : update support condition to im2col

Signed-off-by: Alberto Cabrera <alberto.cabrera@codeplay.com>

* Added TODO to remind supporting FP32 im2col

---------

Signed-off-by: Alberto Cabrera <alberto.cabrera@codeplay.com>
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Nov 18, 2024
* sycl : update support condition to im2col

Signed-off-by: Alberto Cabrera <alberto.cabrera@codeplay.com>

* Added TODO to remind supporting FP32 im2col

---------

Signed-off-by: Alberto Cabrera <alberto.cabrera@codeplay.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ggml changes relating to the ggml tensor library for machine learning SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants