Skip to content

Conversation

@Alcpz
Copy link
Contributor

@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.

@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
Contributor 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
Contributor 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 ggml-org: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 <[email protected]>

* Added TODO to remind supporting FP32 im2col

---------

Signed-off-by: Alberto Cabrera <[email protected]>
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 <[email protected]>

* Added TODO to remind supporting FP32 im2col

---------

Signed-off-by: Alberto Cabrera <[email protected]>
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 <[email protected]>

* Added TODO to remind supporting FP32 im2col

---------

Signed-off-by: Alberto Cabrera <[email protected]>
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.

4 participants