[SYCL][XPTI] Add XPTI notifications for SYCL 2020 images #9770

steffenlarsen · 2023-06-07T13:06:00Z

This commit adds XPTI notifications for SYCL 2020 image classes (sampled_image and unsampled_image) as well as for the associated accessor classes (sampled_image_accessor,
host_sampled_image_accessor, unsampled_image_accessor and host_unsampled_image_accessor).

This commit adds XPTI notifications for SYCL 2020 image classes (`sampled_image` and `unsampled_image`) as well as for the associated accessor classes (`sampled_image_accessor`, `host_sampled_image_accessor`, `unsampled_image_accessor` and `host_unsampled_image_accessor`). Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

sycl/include/sycl/accessor.hpp

sycl/source/detail/image_impl.cpp

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

tovinkere · 2023-06-08T22:49:19Z

sycl/source/detail/xpti_registry.hpp

@@ -65,6 +69,9 @@ class XPTIRegistry {
      // SYCL buffer events
      GBufferStreamID = xptiRegisterStream(SYCL_BUFFER_STREAM_NAME);
      this->initializeStream(SYCL_BUFFER_STREAM_NAME, 0, 1, "0.1");
+      // SYCL image events


@steffenlarsen It is okay to have a new stream for experimental data, but once it is validated, please ensure that the data is emitted in the default "sycl" stream. Everything that is a part of SYCL spec needs to be in the "sycl" stream.

I tried to follow buffers/accessors as closely as possible. Is there some logic I missed that promotes buffers/accessors out the experimental stream or is that a TODO?

@steffenlarsen Having separate streams with common tracepoint types is a reasonable way to do this and limit the explosion of trace point types. When done with validating and settling on the 2020 image implementation, consider moving from "sycl.experimental.image" to "sycl.image"

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

tovinkere · 2023-06-21T00:27:55Z

xpti/include/xpti/xpti_data_types.h

  /// Used to notify that offload buffer will be destructed
-  offload_alloc_destruct = XPTI_TRACE_POINT_BEGIN(22),
+  offload_alloc_buffer_destruct = XPTI_TRACE_POINT_BEGIN(22),


Since we are in an ABI breakage window, can you change this to:
offload_alloc_buffer_destruct = XPTI_TRACE_POINT_END(20),

All pairs, such as [*_begin, _end], [_construct, *_destruct] etc should use XPTI_TRACE_POINT_BEGIN(X)/XPTI_TRACE_POINT_END(X) pairs where the same ID is used for the pair - in this case X.

We have 64 trace _BEGIN/_END pairs we can define here as default. We can extend it with user defined trace types (see documentation). So, we need to be careful about assigning trace_type IDs. If they are truly not scoped pair of trace types, then we may run out of IDs and may have to extend it to use more bits.

So, please consider:
offload_alloc_buffer_associate = XPTI_TRACE_POINT_BEGIN(21),
offload_alloc_buffer_release = XPTI_TRACE_POINT_END(21),

tovinkere · 2023-06-21T00:31:28Z

xpti/include/xpti/xpti_data_types.h

+  /// handle of the offload sampled image
+  offload_alloc_sampled_image_associate = XPTI_TRACE_POINT_BEGIN(27),
+  /// Used to notify that offload sampled image will be destructed
+  offload_alloc_sampled_image_destruct = XPTI_TRACE_POINT_BEGIN(28),


offload_alloc_sampled_image_destruct = XPTI_TRACE_POINT_END(26),

tovinkere · 2023-06-21T00:33:22Z

xpti/include/xpti/xpti_data_types.h

+  /// Used to notify that offload sampled image will be destructed
+  offload_alloc_sampled_image_destruct = XPTI_TRACE_POINT_BEGIN(28),
+  /// Used to notify about releasing internal handle for offload sampled image
+  offload_alloc_sampled_image_release = XPTI_TRACE_POINT_BEGIN(29),


Similarly, _associate/_release should be XPTI_TRACE_POINT_BEGIN(X)/_END(X) if they are semantically efer to the begin and end of the association.
offload_alloc_sampled_image_release = XPTI_TRACE_POINT_BEGIN(27)

tovinkere · 2023-06-21T00:40:37Z

xpti/include/xpti/xpti_data_types.h

+  offload_alloc_unsampled_image_construct = XPTI_TRACE_POINT_BEGIN(30),
+  /// Used to notify about association between user and internal
+  /// handle of the offload unsampled image
+  offload_alloc_unsampled_image_associate = XPTI_TRACE_POINT_BEGIN(31),
+  /// Used to notify that offload unsampled image will be destructed
+  offload_alloc_unsampled_image_destruct = XPTI_TRACE_POINT_BEGIN(32),
+  /// Used to notify about releasing internal handle for offload unsampled image
+  offload_alloc_unsampled_image_release = XPTI_TRACE_POINT_BEGIN(33),


Please consider making the same changes as indicated in previous comments.

tovinkere · 2023-06-21T00:43:05Z

xpti/include/xpti/xpti_data_types.h

+  /// Indicates that the current event is an offload sampled image related
+  offload_sampled_image = XPTI_EVENT(11),
+  /// Indicates that the current event is an offload unsampled image related
+  offload_unsampled_image = XPTI_EVENT(12),
+  /// Indicates that the current event is an offload sampled image accessor
+  /// related
+  offload_sampled_image_accessor = XPTI_EVENT(13),
+  /// Indicates that the current event is an offload sampled image host accessor
+  /// related
+  offload_host_sampled_image_accessor = XPTI_EVENT(14),
+  /// Indicates that the current event is an offload unsampled image accessor
+  /// related
+  offload_unsampled_image_accessor = XPTI_EVENT(15),
+  /// Indicates that the current event is an offload unsampled image host
+  /// accessor related
+  offload_host_unsampled_image_accessor = XPTI_EVENT(16),


The Event types are primarily used to indicate if the "busy" time is attributed to the scheduler, a barrier or an algorithm. For memory operations, this would be "algorithm" as it is needed by an algorithm or we can have a new category called "memory_operation". We don't anything more fine grain than that.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen · 2023-07-06T12:51:46Z

@tovinkere - As discussed a couple weeks ago, I have tried to consolidate the memory-object and accessor enumerators as well as fix the begin and end traces. Please let me know if I missed something or misunderstood something our discussion.

tovinkere

Everything looks good, except the one change requested in xpti_data_types.h
Need to get this in during the ABI breakage window.

tovinkere · 2023-07-06T14:22:53Z

xpti/include/xpti/xpti_data_types.h

@@ -385,21 +386,21 @@ enum class trace_point_type_t : uint16_t {
  mem_release_begin = XPTI_TRACE_POINT_BEGIN(18),
  /// Used to notify that memory has been released.
  mem_release_end = XPTI_TRACE_POINT_END(19),
-  /// Used to notify that offload buffer will be created
-  offload_alloc_construct = XPTI_TRACE_POINT_BEGIN(20),
+  /// Used to notify that offload memory object will be created


@steffenlarsen Since we are consolidating the trace types and fixing issues before the ABI breakage window, can you please also change:
mem_alloc_begin = XPTI_TRACE_POINT_BEGIN(16), mem_alloc_end = XPTI_TRACE_POINT_END(16), mem_release_begin = XPTI_TRACE_POINT_BEGIN(17), mem_release_end = XPTI_TRACE_POINT_END(17),

The rest here look good.

@steffenlarsen Since the ABI breakage window closes today, any updates to this request?

Apologies. It has been addressed now. @stdale-intel - Any chance we can get this in despite the window closing yesterday? It is an XPTI ABI break.

tovinkere · 2023-07-07T18:09:02Z

sycl/source/detail/xpti_registry.hpp

@@ -65,6 +69,9 @@ class XPTIRegistry {
      // SYCL buffer events
      GBufferStreamID = xptiRegisterStream(SYCL_BUFFER_STREAM_NAME);
      this->initializeStream(SYCL_BUFFER_STREAM_NAME, 0, 1, "0.1");
+      // SYCL image events


@steffenlarsen Having separate streams with common tracepoint types is a reasonable way to do this and limit the explosion of trace point types. When done with validating and settling on the 2020 image implementation, consider moving from "sycl.experimental.image" to "sycl.image"

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen · 2023-07-12T07:58:11Z

@intel/llvm-reviewers-runtime | @KseniyaTikhomirova - Friendly ping.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

PietroGhg

Update in the Native CPU test looks good, just FYI #10282 will change the test again, removing the CHECKs that caused you errors

KseniyaTikhomirova

LGTM

tovinkere

LGTM

steffenlarsen · 2023-07-13T08:04:57Z

Spoke to @stdale-intel offline and it should still be fine to merge.

Cached checkout issue for HIP in CI is a known problem.

steffenlarsen requested review from a team as code owners June 7, 2023 13:06

steffenlarsen requested review from KseniyaTikhomirova and tovinkere June 7, 2023 13:06

steffenlarsen commented Jun 7, 2023

View reviewed changes

sycl/include/sycl/accessor.hpp Outdated Show resolved Hide resolved

steffenlarsen commented Jun 7, 2023

View reviewed changes

sycl/source/detail/image_impl.cpp Outdated Show resolved Hide resolved

steffenlarsen temporarily deployed to aws June 7, 2023 15:13 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to aws June 7, 2023 17:09 — with GitHub Actions Inactive

steffenlarsen added 2 commits June 8, 2023 01:31

Add Windows symbols

a87d706

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Merge remote-tracking branch 'intel/sycl' into steffen/image_xpti

641a3e1

steffenlarsen temporarily deployed to aws June 8, 2023 10:40 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to aws June 8, 2023 11:18 — with GitHub Actions Inactive

tovinkere reviewed Jun 8, 2023

View reviewed changes

steffenlarsen added 2 commits June 9, 2023 02:18

Merge remote-tracking branch 'intel/sycl' into steffen/image_xpti

1959f84

Drop string arguments

2b20c43

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen temporarily deployed to aws June 9, 2023 09:59 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to aws June 9, 2023 10:39 — with GitHub Actions Inactive

steffenlarsen requested review from tovinkere and removed request for a team June 20, 2023 11:04

tovinkere requested changes Jun 21, 2023

View reviewed changes

Adjust trace points

fb30575

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen temporarily deployed to aws June 21, 2023 09:24 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to aws June 21, 2023 10:05 — with GitHub Actions Inactive

Merge remote-tracking branch 'intel/sycl' into steffen/image_xpti

44ceac3

steffenlarsen temporarily deployed to aws June 21, 2023 10:37 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to aws June 21, 2023 11:52 — with GitHub Actions Inactive

steffenlarsen added 3 commits July 6, 2023 05:37

Consolidate XPTI trace IDs

949a494

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Remove trace-event work-around after fix

245beb9

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Merge remote-tracking branch 'intel/sycl' into steffen/image_xpti

68e883e

steffenlarsen temporarily deployed to aws July 6, 2023 13:00 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to aws July 6, 2023 15:16 — with GitHub Actions Inactive

tovinkere requested changes Jul 7, 2023

View reviewed changes

Adjust remaining trace begin/end

453dc6f

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen temporarily deployed to aws July 12, 2023 07:42 — with GitHub Actions Inactive

Merge remote-tracking branch 'intel/sycl' into steffen/image_xpti

bb7e501

steffenlarsen temporarily deployed to aws July 12, 2023 08:06 — with GitHub Actions Inactive

Fix kernelhandler-scalar test

049af19

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen requested a review from a team as a code owner July 12, 2023 08:57

steffenlarsen temporarily deployed to aws July 12, 2023 09:11 — with GitHub Actions Inactive

PietroGhg approved these changes Jul 12, 2023

View reviewed changes

steffenlarsen temporarily deployed to aws July 12, 2023 10:01 — with GitHub Actions Inactive

KseniyaTikhomirova approved these changes Jul 12, 2023

View reviewed changes

KseniyaTikhomirova requested a review from tovinkere July 12, 2023 10:52

tovinkere approved these changes Jul 12, 2023

View reviewed changes

steffenlarsen merged commit 8c53527 into intel:sycl Jul 13, 2023

[SYCL][XPTI] Add XPTI notifications for SYCL 2020 images #9770

[SYCL][XPTI] Add XPTI notifications for SYCL 2020 images #9770

Uh oh!

Conversation

steffenlarsen commented Jun 7, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Jul 6, 2023

Uh oh!

tovinkere left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Jul 12, 2023

Uh oh!

PietroGhg left a comment

Choose a reason for hiding this comment

Uh oh!

KseniyaTikhomirova left a comment

Choose a reason for hiding this comment

Uh oh!

tovinkere left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Jul 13, 2023

Uh oh!

Uh oh!

tovinkere left a comment •

edited

Loading