Skip to content

Switch to SPIRV APIs from internal built-in APIs #255

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

Open
wants to merge 6 commits into
base: sycl-develop
Choose a base branch
from

Conversation

jiyang1011
Copy link
Collaborator

sycl_compiler_path=/opt/cutlass/compiler/20250304/
gpu_driver_path=/opt/cutlass/gpu_driver/gfx-driver-ci-comp_igc-28698/extract

This PR is a draft. There is something wrong with SHAPE.
The example pvc_gemm performance dropped a lot.
I will check it
@rolandschulz @mehdi-goli @tdeng5 please take a look

@aacostadiaz
Copy link
Collaborator

Are these the same functions as the ones defined in https://github.khronos.org/SPIRV-Registry/extensions/INTEL/SPV_INTEL_2d_block_io.html ?

@jiyang1011
Copy link
Collaborator Author

Are these the same functions as the ones defined in https://github.khronos.org/SPIRV-Registry/extensions/INTEL/SPV_INTEL_2d_block_io.html ?

Yes

@aacostadiaz
Copy link
Collaborator

Are these the same functions as the ones defined in https://github.khronos.org/SPIRV-Registry/extensions/INTEL/SPV_INTEL_2d_block_io.html ?

Yes

Maybe I'm missing something but the functions in the link use template parameters to define the 2D copy block dimensions, allowing the selection of the internal copy builtin at compile time.

The functions in the PR, however, use runtime variables for this, meaning the selection happens at runtime. This could explain the drop in performance.

@jiyang1011
Copy link
Collaborator Author

jiyang1011 commented Mar 18, 2025

Are these the same functions as the ones defined in https://github.khronos.org/SPIRV-Registry/extensions/INTEL/SPV_INTEL_2d_block_io.html ?

Yes

Maybe I'm missing something but the functions in the link use template parameters to define the 2D copy block dimensions, allowing the selection of the internal copy builtin at compile time.

The functions in the PR, however, use runtime variables for this, meaning the selection happens at runtime. This could explain the drop in performance.

In the spec, it is just an example to show the semantics, but it is not an API definition. Spec defines only SPIRV API. And I find prefetch functionality is the root cause, which seems not working well.

@tdeng5 tdeng5 changed the title spirv APIs Switch to SPIRV APIs instead of internal built-in API Mar 18, 2025
@tdeng5 tdeng5 changed the title Switch to SPIRV APIs instead of internal built-in API Switch to SPIRV APIs from internal built-in APIs Mar 18, 2025
@aacostadiaz
Copy link
Collaborator

aacostadiaz commented Mar 18, 2025

@jiyang1011 Could you try adding something like 36aa2d4 to use SPIRV or the builtins depending on the compiler version? We might need to disable SPIRV before merging until we can update CI with an IGC version that includes the SPIRV definitions.

@jiyang1011
Copy link
Collaborator Author

@jiyang1011 Could you try adding something like 36aa2d4 to use SPIRV or the builtins depending on the compiler version? We might need to disable SPIRV before merging until we can update CI with an IGC version that includes the SPIRV definitions.

OK

@jiyang1011 jiyang1011 force-pushed the jiyang/spirv_api branch 3 times, most recently from 729e553 to aa532b3 Compare March 25, 2025 05:21
@jiyang1011
Copy link
Collaborator Author

@aacostadiaz

@joeatodd
Copy link
Collaborator

Hey @jiyang1011 👋 I'm struggling to compile e.g. pvc_gemm.cpp with this PR. I am using the 2025-03-24 nightly build of DPC++ and version 28698 of IGC. I get IGC: Internal Compiler Error: Segmentation violation. Can you help?

Copy link
Collaborator

@joeatodd joeatodd left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I have a few suggestions for improvements 👍

@aacostadiaz
Copy link
Collaborator

Hey @jiyang1011 👋 I'm struggling to compile e.g. pvc_gemm.cpp with this PR. I am using the 2025-03-24 nightly build of DPC++ and version 28698 of IGC. I get IGC: Internal Compiler Error: Segmentation violation. Can you help?

I made some changes in https://github.com/aacostadiaz/cutlass-fork/tree/aacosta/spirv_api to make the code compile and run with IGC 28698. There is an issue with the SPIRV functions for MMA, so I had to disable them. I also enabled prefetch to always use the builtins, which restored the performance of the GEMM example. I haven't checked all configurations in the benchmark.

@jiyang1011 Could you take a look and consider adding these changes to this PR? Do you know what the issue is with the SPIRV functions for MMA? Do we need a different version of IGC for it to work?

@jiyang1011 jiyang1011 force-pushed the jiyang/spirv_api branch 2 times, most recently from d9b8248 to 0cdbb6a Compare March 26, 2025 05:25
@jiyang1011
Copy link
Collaborator Author

Hey @jiyang1011 👋 I'm struggling to compile e.g. pvc_gemm.cpp with this PR. I am using the 2025-03-24 nightly build of DPC++ and version 28698 of IGC. I get IGC: Internal Compiler Error: Segmentation violation. Can you help?

I made some changes in https://github.com/aacostadiaz/cutlass-fork/tree/aacosta/spirv_api to make the code compile and run with IGC 28698. There is an issue with the SPIRV functions for MMA, so I had to disable them. I also enabled prefetch to always use the builtins, which restored the performance of the GEMM example. I haven't checked all configurations in the benchmark.

@jiyang1011 Could you take a look and consider adding these changes to this PR? Do you know what the issue is with the SPIRV functions for MMA? Do we need a different version of IGC for it to work?

I think the include chain makes the compilation failure maybe. I reorged it following @joeatodd 's suggestion. 2025-03-24 nightly build of DPC++ and version 28698 of IGC. are OK

@jiyang1011 jiyang1011 requested a review from taozha2 March 28, 2025 00:33
@jiyang1011 jiyang1011 force-pushed the jiyang/spirv_api branch 3 times, most recently from 969eafe to f57f308 Compare April 1, 2025 01:29
@jiyang1011
Copy link
Collaborator Author

hi @aacostadiaz Could you help me debug the CI. I have looked into the info of CI, but I have no clue where the error is

@aacostadiaz
Copy link
Collaborator

hi @aacostadiaz Could you help me debug the CI. I have looked into the info of CI, but I have no clue where the error is

It looks like the current version of IGC still doesn't have the SPIRV functions. We will need to disable SPIRV until it is available. This is IGC version 1.6.32224+14 in CI.

cc @rolandschulz

@jiyang1011 jiyang1011 force-pushed the jiyang/spirv_api branch 3 times, most recently from 795f6ca to dd576ec Compare April 8, 2025 01:38
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.

5 participants