[SYCL] Fix UserData usage for XPTIScope #11091

KseniyaTikhomirova · 2023-09-06T15:51:38Z

Aligned node_create usage with https://github.com/intel/llvm/blob/sycl/sycl/doc/design/SYCLInstrumentationUsingXPTI.md

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

KseniyaTikhomirova · 2023-09-13T12:46:28Z

hip failure is reported here #10460

tovinkere · 2023-09-13T15:05:17Z

sycl/source/detail/queue_impl.cpp

@@ -67,7 +67,7 @@ event queue_impl::memset(const std::shared_ptr<detail::queue_impl> &Self,
  // information
  XPTIScope PrepareNotify((void *)this,
                          (uint16_t)xpti::trace_point_type_t::node_create,
-                          SYCL_MEM_ALLOC_STREAM_NAME, "queue.memset()");
+                          SYCL_STREAM_NAME, "memory_transfer_node");


I am not sure "memory_transfer_node" is clear as we lose information that it was a memset() operation.

memset and memcpy has different metadata for source: pointer or value. If it is needed we could add new node_create user_data type for memset and declare it at the design doc. Anyway - current "queue.memset" usage is invalid and should be fixed.

tovinkere · 2023-09-13T15:06:19Z

sycl/source/detail/queue_impl.cpp

@@ -143,7 +143,7 @@ event queue_impl::memcpy(const std::shared_ptr<detail::queue_impl> &Self,
  // pointer.
  XPTIScope PrepareNotify((void *)this,
                          (uint16_t)xpti::trace_point_type_t::node_create,
-                          SYCL_MEM_ALLOC_STREAM_NAME, "queue.memcpy()");
+                          SYCL_STREAM_NAME, "memory_transfer_node");


See above comment - we seem to have lost information. Are memset/memcpy being recorded elsewhere, perhaps in metadata? I am assuming this was a cosmetic change and not related to HIP failure?

it is not related to the HIP. Customer has reported the issue that after code_location enabling node_create signal for queue methods started to report different data and I found a few gaps here, this implementation is not aligned with design doc for node_create trace.

tovinkere · 2023-09-13T15:10:54Z

sycl/source/detail/xpti_registry.hpp

-    if (MTraceType == (uint16_t)xpti::trace_point_type_t::graph_create ||
-        MTraceType == (uint16_t)xpti::trace_point_type_t::node_create ||
-        MTraceType == (uint16_t)xpti::trace_point_type_t::edge_create)
+    MTP = new TracePoint(TData.fileName(), TData.functionName(),


Since the contract regarding code_location has been changed, this will work for this use-case. We may need to look at how code_location is being used and may have to address it for all cases.

Context:

Previous code_location had all attributes set to NULL when NDEBUG was used and the previous code here handled it.

New definition of code_location only zeroes out file_location when NDEBUG is used, which changed the contract code_location had with all previous use

"may have to address it for all cases."
do you mean to handle the case with empty code_location data? Please correct me if I am wrong, I didn't understand why UserData usage is better there because it will be the same for all queue.memset calls and will not bring any benefits. If code_location is not available the only hope to distinguish calls is CodePtr which is still used, doesn't it?

As long as the metadata includes the node_create was for memset or memcpy, we are good here.

Regarding the change in code_location that now has the function name at all times, we may need to look at all uses of FunctionName and address it in a consistent manner. The above example is a case were it was trying to handle the case when all attributes of code_location were set to nullptr, and there may be more instances where this cleanup can be applied.

@tovinkere got it, thanks for the clarification. I walked through the SYCL RT code today and it looks like we do not have any code_location absence WAs left.

tovinkere

LGTM

againull · 2023-09-14T17:48:36Z

@KseniyaTikhomirova HIP failure doesn't look related to your changes. But I am not sure about failure in linux gen12 testing:

Failed Tests (1):
SYCL :: XPTI/basic_event_collection_linux.cpp
Can it be related to your changes?

KseniyaTikhomirova · 2023-09-15T09:02:16Z

@KseniyaTikhomirova HIP failure doesn't look related to your changes. But I am not sure about failure in linux gen12 testing:

Failed Tests (1): SYCL :: XPTI/basic_event_collection_linux.cpp Can it be related to your changes?

seems to be related, will fix today

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

KseniyaTikhomirova · 2023-09-15T12:19:30Z

@KseniyaTikhomirova HIP failure doesn't look related to your changes. But I am not sure about failure in linux gen12 testing:
Failed Tests (1): SYCL :: XPTI/basic_event_collection_linux.cpp Can it be related to your changes?

seems to be related, will fix today

done. previously node_create & edge_create seems to have one event and metadata was mixed.

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

KseniyaTikhomirova · 2023-09-26T12:16:15Z

failures on win are the same as reported here #11224

aelovikov-intel · 2023-09-26T14:46:33Z

failures on win are the same as reported here #11224

From Windows log:

warning: reached timeout, skipping remaining tests

Also, this PR's CI run seems to have had the exact problem we've been struggling with our Win CI.

Fix UserData usage for XPTIScope

39b7485

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

KseniyaTikhomirova requested a review from a team as a code owner September 6, 2023 15:51

KseniyaTikhomirova requested a review from againull September 6, 2023 15:51

KseniyaTikhomirova marked this pull request as draft September 7, 2023 08:57

KseniyaTikhomirova added 2 commits September 13, 2023 04:50

align impl with design docs

082b309

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

add unit test

fcacffa

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

KseniyaTikhomirova marked this pull request as ready for review September 13, 2023 12:17

KseniyaTikhomirova requested a review from tovinkere September 13, 2023 12:17

tovinkere reviewed Sep 13, 2023

View reviewed changes

KseniyaTikhomirova requested a review from tovinkere September 14, 2023 12:22

tovinkere approved these changes Sep 14, 2023

View reviewed changes

againull approved these changes Sep 14, 2023

View reviewed changes

KseniyaTikhomirova added 2 commits September 15, 2023 02:19

Merge branch 'sycl' into xpti_fix

47ebe4a

remove line generated by event handling gap

a76cd32

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

fix test

9de896c

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

Merge branch 'sycl' into xpti_fix

5c7c121

KseniyaTikhomirova temporarily deployed to WindowsCILock September 27, 2023 12:22 — with GitHub Actions Inactive

KseniyaTikhomirova temporarily deployed to WindowsCILock September 27, 2023 12:45 — with GitHub Actions Inactive

sergey-semenov merged commit f92e432 into intel:sycl Sep 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Fix UserData usage for XPTIScope #11091

[SYCL] Fix UserData usage for XPTIScope #11091

KseniyaTikhomirova commented Sep 6, 2023 •

edited

Loading

KseniyaTikhomirova commented Sep 13, 2023

tovinkere Sep 13, 2023

KseniyaTikhomirova Sep 13, 2023

tovinkere Sep 13, 2023

KseniyaTikhomirova Sep 13, 2023

tovinkere Sep 13, 2023

KseniyaTikhomirova Sep 13, 2023

tovinkere Sep 13, 2023 •

edited

Loading

KseniyaTikhomirova Sep 14, 2023 •

edited

Loading

tovinkere left a comment

againull commented Sep 14, 2023

KseniyaTikhomirova commented Sep 15, 2023

KseniyaTikhomirova commented Sep 15, 2023

KseniyaTikhomirova commented Sep 26, 2023 •

edited

Loading

aelovikov-intel commented Sep 26, 2023 •

edited

Loading

[SYCL] Fix UserData usage for XPTIScope #11091

[SYCL] Fix UserData usage for XPTIScope #11091

Conversation

KseniyaTikhomirova commented Sep 6, 2023 • edited Loading

KseniyaTikhomirova commented Sep 13, 2023

tovinkere Sep 13, 2023

Choose a reason for hiding this comment

KseniyaTikhomirova Sep 13, 2023

Choose a reason for hiding this comment

tovinkere Sep 13, 2023

Choose a reason for hiding this comment

KseniyaTikhomirova Sep 13, 2023

Choose a reason for hiding this comment

tovinkere Sep 13, 2023

Choose a reason for hiding this comment

KseniyaTikhomirova Sep 13, 2023

Choose a reason for hiding this comment

tovinkere Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

KseniyaTikhomirova Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

tovinkere left a comment

Choose a reason for hiding this comment

againull commented Sep 14, 2023

KseniyaTikhomirova commented Sep 15, 2023

KseniyaTikhomirova commented Sep 15, 2023

KseniyaTikhomirova commented Sep 26, 2023 • edited Loading

aelovikov-intel commented Sep 26, 2023 • edited Loading

KseniyaTikhomirova commented Sep 6, 2023 •

edited

Loading

tovinkere Sep 13, 2023 •

edited

Loading

KseniyaTikhomirova Sep 14, 2023 •

edited

Loading

KseniyaTikhomirova commented Sep 26, 2023 •

edited

Loading

aelovikov-intel commented Sep 26, 2023 •

edited

Loading