Skip to content

[SYCL] Add esimd device descriptor for 2d load/store/prefetch #15905

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

Merged
merged 11 commits into from
Nov 28, 2024

Conversation

againull
Copy link
Contributor

@againull againull commented Oct 28, 2024

Add esimd device descriptor to check if 2d block operations are supported by the device.
UR counterpart: oneapi-src/unified-runtime#2261

@againull againull force-pushed the 2d_block_array_aspects branch 2 times, most recently from b7ba417 to a3bd516 Compare October 28, 2024 23:45
@againull againull force-pushed the 2d_block_array_aspects branch from a3bd516 to 6aac1e2 Compare October 30, 2024 04:41
@againull againull marked this pull request as ready for review October 30, 2024 17:31
@againull againull requested review from a team as code owners October 30, 2024 17:31
@againull againull requested a review from bso-intel October 30, 2024 17:31
@Pennycook
Copy link
Contributor

Pennycook commented Oct 30, 2024

Am I right in thinking that this feature (2D block loads/stores) is not actually exposed to SYCL?

@rolandschulz
Copy link
Contributor

It is indirectly available to users through the joint_matrix abstraction. But for that I don't think the user needs to be able to query it. Currently it isn't available directly in a supported way (cutlass uses IGC builtins). But we plan to support it at least through SPIR-V/OpenCL interop and if there demand also as SYCL extension.

@gmlueck
Copy link
Contributor

gmlueck commented Oct 30, 2024

It does seem weird to add a SYCL query for a feature that cannot be used via SYCL. Can we instead add the query at the same level as the API? For example, if applications will access 2D block load/store via OpenCL, can we add an OpenCL query?

@againull
Copy link
Contributor Author

Thanks for the comments, the request was from the user to add this query at the SYCL level. I will get more details about how they use 2D block load/store feature and if they can consider using L0 API directly.

@againull againull marked this pull request as draft October 30, 2024 18:58
@againull againull force-pushed the 2d_block_array_aspects branch from 6aac1e2 to 37044fc Compare November 19, 2024 22:32
@againull againull force-pushed the 2d_block_array_aspects branch from 37044fc to eceb3b7 Compare November 19, 2024 22:34
@againull
Copy link
Contributor Author

againull commented Nov 19, 2024

According to the info from the user they use either esimd or joint_matrix.
So, I believe if someone uses joint_matrix operations then they can check corresponding matrix aspects.

In case of ESIMD it looks like load_2d/store_2d operations are mapped to this low level feature https://github.com/intel/compute-runtime/blob/24.39.31294.12/level_zero/doc/experimental_extensions/2D_BLOCK_TRANSPOSE.md.
@sarnex Could you please help to confirm that or maybe recommend someone who can help.

Currently in the ESIMD docs there is a statement that load_2d/store_2d can be used only on PVC (see https://github.com/intel/llvm/pull/15905/files#diff-fef71b0090d0ea44c96240cd9d6497881b617b91e234635a19313eb69864b7f3L555):
I believe we can create a new esimd aspect (which checks if device has 2d block load/store) for this load_2d/store_2d operations.
So that users don't have to check particular devices but can just check the aspect.

Does it make sense to introduce such aspect? I've updated the PR with proposed changes.

@sarnex
Copy link
Contributor

sarnex commented Nov 19, 2024

I can't say for sure if the architecture restriction is correct (idk if 2d ops are PVC only, or actually PVC or later or what).
But I have no flags to adding an aspect, I guess it would just come down to a arch/device ID check in UR somewhere.
I can help, but please let me know what I'm signing up for.

@againull againull force-pushed the 2d_block_array_aspects branch from d671fcb to cc522a6 Compare November 21, 2024 22:35
@againull againull changed the title [SYCL] Add aspects to check if Intel GPU supports 2D block array operations [SYCL] Add esimd device descriptor for 2d load/store/prefetch Nov 21, 2024
@againull againull force-pushed the 2d_block_array_aspects branch from cc522a6 to 08bbd1c Compare November 21, 2024 22:37
@againull
Copy link
Contributor Author

againull commented Nov 22, 2024

dyn_cgf_different_arg_nums.cpp failure on CUDA is unrelated and is fixed here: #16157

@againull
Copy link
Contributor Author

@intel/unified-runtime-reviewers Could you please approve.

@againull againull merged commit a024380 into intel:sycl Nov 28, 2024
15 checks passed
@AlexeySachkov
Copy link
Contributor

@againull, FYI, this PR breaks some tests on PVC:

  • ESIMD/dpas/dpas_tf32.cpp
  • ESIMD/acc_gather_scatter_rgba_stateless_64.cpp
  • ESIMD/lsc/lsc_block_load_store_stateless_flag_64.cpp
  • ESIMD/lsc/lsc_block_load_store_stateless_64.cpp
  • ESIMD/accessor_stateless_ctor_64.cpp
  • ESIMD/accessor_load_store_stateless_64.cpp
  • ESIMD/lsc/lsc_gather_scatter_stateless_64.cpp
  • ESIMD/lsc/lsc_load_store_2d_compare.cpp
  • ESIMD/accessor_gather_scatter_stateless_64.cpp
  • ESIMD/accessor_stateless_64.cpp
  • ESIMD/slm_init_no_inline.cpp

@againull
Copy link
Contributor Author

againull commented Dec 2, 2024

@againull, FYI, this PR breaks some tests on PVC:

  • ESIMD/dpas/dpas_tf32.cpp
  • ESIMD/acc_gather_scatter_rgba_stateless_64.cpp
  • ESIMD/lsc/lsc_block_load_store_stateless_flag_64.cpp
  • ESIMD/lsc/lsc_block_load_store_stateless_64.cpp
  • ESIMD/accessor_stateless_ctor_64.cpp
  • ESIMD/accessor_load_store_stateless_64.cpp
  • ESIMD/lsc/lsc_gather_scatter_stateless_64.cpp
  • ESIMD/lsc/lsc_load_store_2d_compare.cpp
  • ESIMD/accessor_gather_scatter_stateless_64.cpp
  • ESIMD/accessor_stateless_64.cpp
  • ESIMD/slm_init_no_inline.cpp

Thanks, those have requires: pvc, so I didn't get failures in pre/post-commit. I know the rootcause, will fix.

againull added a commit that referenced this pull request Dec 4, 2024
… tests (#16254)

Follow-up after #15905
These tests use `using` directive for both `sycl` and `esimd` namespaces
and `info` is available in both, so we have to explicitly specify
namespace in this case to avoid ambiguity.

Hasn't been noticed before because tests run only on pvc.
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.

8 participants