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][FPGA] Implement task_sequence header, properties, and add new fpga_cluster kernel property #12453

Merged

Conversation

bowenxue-intel
Copy link
Contributor

@bowenxue-intel bowenxue-intel commented Jan 19, 2024

Implement task_sequence header, task_sequence properties, and fpga_cluster kernel property according to the spec updates #6348 (almost ready to be merged)
Header

  • Refactor invocation and response capacity to be on the Create intrinsic, so Async and Get do not take in respective argument
  • Remove max_outstanding argument of Create intrinsic
  • Add pipelined and fpga_cluster arguments to Create intrinsic
  • Async, Get, and Release intrinsic now accept the TargetExtType target("spirv.TaskSequenceINTEL") returned from the Create intrinsic, and do not take in object pointer or task function pointer arguments
  • Task Sequence accepts properties

Task Sequence Properties

  • Convert invocation and response capacity from template parameters to properties
  • Add balanced property to remove Get intrinsic loop in destructor

FPGA Kernel properties

  • Add fpga_cluster property
  • Support fpga_cluster and pipelined property for task sequence

Copy link
Contributor

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 90f03ee7c82ed9c6f2e3c34d300e6c4b18fe1d67 f1ee83f32085094204d6715f38bdc664cc8f1906 -- sycl/include/sycl/ext/intel/experimental/task_sequence/properties.hpp sycl/include/sycl/ext/intel/experimental/task_sequence/task_sequence.hpp llvm/lib/SYCLLowerIR/CompileTimePropertiesPass.cpp sycl/include/CL/__spirv/spirv_ops.hpp sycl/include/sycl/ext/intel/experimental/fpga_kernel_properties.hpp sycl/include/sycl/ext/oneapi/properties/property.hpp sycl/test/extensions/properties/properties_kernel_fpga.cpp
View the diff from clang-format here.
diff --git a/sycl/include/CL/__spirv/spirv_ops.hpp b/sycl/include/CL/__spirv/spirv_ops.hpp
index 0419bc974f..6ddac0ebf6 100644
--- a/sycl/include/CL/__spirv/spirv_ops.hpp
+++ b/sycl/include/CL/__spirv/spirv_ops.hpp
@@ -995,8 +995,8 @@ __spirv_ConvertFToBF16INTEL(float) noexcept;
 extern __DPCPP_SYCL_EXTERNAL float
     __spirv_ConvertBF16ToFINTEL(uint16_t) noexcept;
 
-__SYCL_CONVERGENT__ extern __DPCPP_SYCL_EXTERNAL __SYCL_EXPORT
-    __ocl_vec_t<uint32_t, 4>
+__SYCL_CONVERGENT__ extern __DPCPP_SYCL_EXTERNAL
+    __SYCL_EXPORT __ocl_vec_t<uint32_t, 4>
     __spirv_GroupNonUniformBallot(uint32_t Execution, bool Predicate) noexcept;
 
 // TODO: I'm not 100% sure that these NonUniform instructions should be

Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

Please add a clang test for the spirv type genrated. You may modify the one from #12820 if that is easier.

@bowenxue-intel
Copy link
Contributor Author

Please add a clang test for the spirv type genrated. You may modify the one from #12820 if that is easier.

Added. There are some issues with my clang environment related to finding the correct gcc directory that is preventing me from using llvm-lit directly to check whether the LIT tests are passing. I'm basing the test CHECK conditions on manual clang++ compiles so the tests may require a bit more tweaking.

@bowenxue-intel
Copy link
Contributor Author

@intel/dpcpp-tools-reviewers Can someone please review this PR? This is needed for the FPGA team's next release.

@bowenxue-intel
Copy link
Contributor Author

@intel/llvm-gatekeepers PR has been fully approved, please merge whenever possible.

@steffenlarsen
Copy link
Contributor

@intel/llvm-gatekeepers PR has been fully approved, please merge whenever possible.

There seem to be compilation issues.

@bowenxue-intel
Copy link
Contributor Author

@intel/llvm-gatekeepers PR has been fully approved, please merge whenever possible.

There seem to be compilation issues.

Apologies, bad attempt after resolving the merge conflict through Github UI. Should be fixed now.

@dm-vodopyanov dm-vodopyanov merged commit 7b9001e into intel:sycl Mar 12, 2024
11 checks passed
@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Mar 20, 2024

Tests introduced here have been failing for more than a week in post-commit. @bowenxue-intel , @dm-vodopyanov , please FIX ASAP.

@dm-vodopyanov , I want to remind that it's the gatekeeper's responsibility to watch for post-commit failures after PR is merged and work with author to address any failures.

  SYCL :: TaskSequence/concurrent-loops.cpp
  SYCL :: TaskSequence/in-order-async-get.cpp
  SYCL :: TaskSequence/mult-and-add.cpp
  SYCL :: TaskSequence/multi-kernel-task-function-reuse.cpp
  SYCL :: TaskSequence/producer-consumer.cpp
  SYCL :: TaskSequence/struct-array-args-and-return.cpp

aejjehint added a commit to aejjehint/intel--llvm that referenced this pull request Jun 28, 2024
…add new fpga_cluster kernel property (intel#12453)"

This reverts commit 7b9001e.
aejjehint added a commit to aejjehint/intel--llvm that referenced this pull request Jul 3, 2024
…add new fpga_cluster kernel property (intel#12453)"

This reverts commit 7b9001e.
aejjehint added a commit to aejjehint/intel--llvm that referenced this pull request Jul 9, 2024
…add new fpga_cluster kernel property (intel#12453)"

This reverts commit 7b9001e.
steffenlarsen pushed a commit that referenced this pull request Jul 11, 2024
This reverts PR #12453 and #13080

The original PR introducing task sequence was checked in prematurely as
the backend support isn't in yet. When the backend support is in we will
re-submit this change.
againull pushed a commit that referenced this pull request Jul 26, 2024
This change re-apply the task sequence changes from PR #12453 that were
reverted in #14359 now that the spec has been approved and the back-end
implementation is ready. The spec is also moved from proposed to
experimental.

---------

Co-authored-by: bowenxue-intel <bowen.xue@intel.com>
Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>
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.

10 participants