[SYCL][XPTI] Revisit resource management strategy #4494

alexbatashev · 2021-09-04T12:48:21Z

Global objects are typically destroyed on process tear down, but the order of the destructor calls is undefined. Since SYCL applications can potentially call SYCL APIs from global context, XPTI and all of its resources have to outlive user application. This patch refactors XPTI proxy library and framework to allocate global objects on heap and manages their lifetime based on communications from traced application.

Each user of XPTI (e.g. SYCL runtime) has to call xptiFrameworkInitialize() once prior to any other XPTI API. When application is done collecting trace information, it must close streams and then call xptiFrameworkFinalize().

The XPTI framework will maintain a reference counter, and will only free resources and unload libraries, when the counter hits 0. This will allow the subscribers to survive past DllMain call or global shared library destructor.

alexbatashev · 2021-09-04T12:49:28Z

This PR depends on #4462

sycl/source/detail/pi.cpp

xpti/src/xpti_proxy.cpp

smaslov-intel

LGTM overall, just copy-past errors that I pointed in comments. This also suggests that proper testing is missing, since this wasn't noted before.

alexbatashev · 2021-09-10T06:39:23Z

This also suggests that proper testing is missing, since this wasn't noted before.

Right, tests were disabled for a long time and are hardly buildable right now due to warnings implied by compiler upgrades. I'm working on cleaning that up and enabling it in CI. I'll convert this PR to draft till tests are merged.

UPD: tests for this patch are included into intel/llvm-test-suite#458, which depends on #4545

xptifw/src/xpti_trace_framework.cpp

smaslov-intel

LGTM

andykaylor · 2021-09-21T20:26:05Z

sycl/source/detail/xpti_registry.hpp

@@ -31,6 +31,15 @@ inline constexpr const char *SYCL_PIDEBUGCALL_STREAM_NAME = "sycl.pi.debug";

 class XPTIRegistry {
 public:
+  void initializeFrameworkOnce() {
+#ifdef XPTI_ENABLE_INSTRUMENTATION
+    if (!MInitialized) {


Does this need a thread guard to avoid race conditions?

Correct. Right now this is only called from a single place, which is guarded std::call_once, but the future instrumentations will initialize streams closer to use place, so that we don't notify tools about streams we're not going to use.

I have replaced this with std::call_once, it better demonstrates my intentions and is definitely thread safe.

sycl/source/detail/xpti_registry.hpp

xpti/include/xpti_trace_framework.hpp

xptifw/src/xpti_trace_framework.cpp

andykaylor

lgtm

…_revisited

alexbatashev · 2021-09-29T18:38:23Z

@andykaylor @smaslov-intel I fixed merge conflicts, could you please re-review?

* sycl: (108 commits) [SYCL][XPTI] Revisit resource management strategy (intel#4494) [SYCL][ESIMD] Fix misprint: ESIMD_L1_FLUASH_RO_DATA -> ESIMD_L1_FLUSH_RO_DATA (intel#4681) [SYCL] Make kernel_bundle interop more conformant (intel#4672) [SYCL] Submission with kernel parameter ignores set kernel bundle (intel#4667) [SYCL] Add support for std::byte to vec class (intel#4637) [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#4671) [SYCL] Fix an error on host when big image is used on opencl:gpu (intel#4668) [SYCL] Exclude exported symbols from kernel bundles (intel#4660) Revert "[SYCL] Allow overriding plugin libraries (intel#4067)" (intel#4659) [SYCL] Handle exceptions on mutually exclusive handler operations (intel#4639) [sycl-post-link] Don't split module if function pointer has a user that's not CallInst (intel#4657) [SYCL][HIP] Fix MemBufferFill for nvidia platform (intel#4629) [SYCL][Doc] Describe DPC++ CUDA install w/ non-standard toolkit loc (intel#4663) [SYCL] Fix device code instrumentation (intel#4615) Remove myself as a code owner (intel#4653) [SYCL] Fix overwriting insert to sub_group_mask (intel#4656) [x86][Matrix] Replace packed_a with row_major in matrix testcases (intel#4641) [SYCL][Doc] Add device global extension spec (intel#4233) [sycl-post-link] Adds property listing exported functions (intel#4626) [Driver][SYCL] Do not consider non-archive files for FPGA binary checks (intel#4644) ...

* sycl: (107 commits) [SYCL][XPTI] Revisit resource management strategy (intel#4494) [SYCL][ESIMD] Fix misprint: ESIMD_L1_FLUASH_RO_DATA -> ESIMD_L1_FLUSH_RO_DATA (intel#4681) [SYCL] Make kernel_bundle interop more conformant (intel#4672) [SYCL] Submission with kernel parameter ignores set kernel bundle (intel#4667) [SYCL] Add support for std::byte to vec class (intel#4637) [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#4671) [SYCL] Fix an error on host when big image is used on opencl:gpu (intel#4668) [SYCL] Exclude exported symbols from kernel bundles (intel#4660) Revert "[SYCL] Allow overriding plugin libraries (intel#4067)" (intel#4659) [SYCL] Handle exceptions on mutually exclusive handler operations (intel#4639) [sycl-post-link] Don't split module if function pointer has a user that's not CallInst (intel#4657) [SYCL][HIP] Fix MemBufferFill for nvidia platform (intel#4629) [SYCL][Doc] Describe DPC++ CUDA install w/ non-standard toolkit loc (intel#4663) [SYCL] Fix device code instrumentation (intel#4615) Remove myself as a code owner (intel#4653) [SYCL] Fix overwriting insert to sub_group_mask (intel#4656) [x86][Matrix] Replace packed_a with row_major in matrix testcases (intel#4641) [SYCL][Doc] Add device global extension spec (intel#4233) [sycl-post-link] Adds property listing exported functions (intel#4626) [Driver][SYCL] Do not consider non-archive files for FPGA binary checks (intel#4644) ...

This reverts commit c91b3b8.

…tel#4494)"" This reverts commit f45c1e7.

alexbatashev requested a review from tovinkere September 4, 2021 12:48

alexbatashev added 2 commits September 5, 2021 16:50

[SYCL][XPTI] Revisit resource management strategy

51f5258

fix failures

73115fa

alexbatashev force-pushed the xpti_global_objects_revisited branch from 4da67e0 to 73115fa Compare September 5, 2021 13:52

alexbatashev marked this pull request as ready for review September 5, 2021 13:53

alexbatashev requested review from andykaylor, smaslov-intel and a team as code owners September 5, 2021 13:53

smaslov-intel reviewed Sep 9, 2021

View reviewed changes

sycl/source/detail/pi.cpp Show resolved Hide resolved

smaslov-intel reviewed Sep 9, 2021

View reviewed changes

xpti/src/xpti_proxy.cpp Outdated Show resolved Hide resolved

smaslov-intel reviewed Sep 9, 2021

View reviewed changes

xpti/src/xpti_proxy.cpp Outdated Show resolved Hide resolved

smaslov-intel suggested changes Sep 9, 2021

View reviewed changes

alexbatashev marked this pull request as draft September 10, 2021 06:39

alexbatashev added 2 commits September 12, 2021 10:37

fix a few bugs

a706b8a

more fixes

86111c7

This was referenced Sep 12, 2021

[SYCL] Introduce sycl-sanitizer tool #4174

Closed

[SYCL][XPTI] Enable XPTI and XPTI Framework E2E tests intel/llvm-test-suite#458

Merged

alexbatashev requested a review from smaslov-intel September 12, 2021 15:30

alexbatashev marked this pull request as ready for review September 12, 2021 15:31

smaslov-intel reviewed Sep 13, 2021

View reviewed changes

xptifw/src/xpti_trace_framework.cpp Show resolved Hide resolved

smaslov-intel reviewed Sep 13, 2021

View reviewed changes

xptifw/src/xpti_trace_framework.cpp Outdated Show resolved Hide resolved

address some feedback

ae1184d

alexbatashev requested a review from smaslov-intel September 18, 2021 11:42

smaslov-intel previously approved these changes Sep 20, 2021

View reviewed changes

andykaylor reviewed Sep 21, 2021

View reviewed changes

alexbatashev dismissed smaslov-intel’s stale review via 10546f1 September 22, 2021 18:45

use std::call_once

10546f1

alexbatashev requested a review from andykaylor September 22, 2021 18:56

andykaylor previously approved these changes Sep 23, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/sycl' into xpti_global_objects…

028beae

…_revisited

alexbatashev dismissed andykaylor’s stale review via 028beae September 29, 2021 18:36

smaslov-intel approved these changes Oct 1, 2021

View reviewed changes

andykaylor approved these changes Oct 2, 2021

View reviewed changes

dm-vodopyanov merged commit c91b3b8 into intel:sycl Oct 2, 2021

vmaksimo pushed a commit to vmaksimo/llvm that referenced this pull request Oct 4, 2021

Revert "[SYCL][XPTI] Revisit resource management strategy (intel#4494)"

f45c1e7

This reverts commit c91b3b8.

vmaksimo pushed a commit to vmaksimo/llvm that referenced this pull request Nov 19, 2021

Revert "Revert "[SYCL][XPTI] Revisit resource management strategy (in…

567e578

…tel#4494)"" This reverts commit f45c1e7.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL][XPTI] Revisit resource management strategy #4494

[SYCL][XPTI] Revisit resource management strategy #4494

Uh oh!

alexbatashev commented Sep 4, 2021

Uh oh!

alexbatashev commented Sep 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smaslov-intel left a comment

Uh oh!

alexbatashev commented Sep 10, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

smaslov-intel left a comment

Uh oh!

andykaylor Sep 21, 2021

Uh oh!

alexbatashev Sep 22, 2021

Uh oh!

alexbatashev Sep 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andykaylor left a comment

Uh oh!

alexbatashev commented Sep 29, 2021

Uh oh!

Uh oh!

[SYCL][XPTI] Revisit resource management strategy #4494

[SYCL][XPTI] Revisit resource management strategy #4494

Uh oh!

Conversation

alexbatashev commented Sep 4, 2021

Uh oh!

alexbatashev commented Sep 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

alexbatashev commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

andykaylor Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

alexbatashev Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

alexbatashev Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andykaylor left a comment

Choose a reason for hiding this comment

Uh oh!

alexbatashev commented Sep 29, 2021

Uh oh!

Uh oh!

alexbatashev commented Sep 10, 2021 •

edited

Loading