[SYCL] Instrumentation for application profiling #1129

tovinkere · 2020-02-13T19:08:46Z

Overview

SYCL runtime is instrumented for capturing the semantic and trace information of the asynchronous task graph.
For all data parallel constructs, end user code-location information is captured and preserved as metadata and made available to the tracing framework.
Uses a CMake configuration to turn this instrumentation on and it is ON by default.
Uses xpti proxy library for instrumentation and the proxy library is available under llvm/xpti. If the dynamic portion of the instrumentation framework is not available, then tracing gets disabled. Tracing can also be disabled by using environment variables as well.
Documentation on what is being captured is available under llvm/xpti/doc.

alexbatashev

A general comment. Am I right that users need to add some kind of definition to their command lines? Can you please document them in user guide? Should we also create a more user friendly alias for clang driver?

sycl/include/CL/sycl/detail/cg.hpp

tovinkere · 2020-02-18T14:10:31Z

A general comment. Am I right that users need to add some kind of definition to their command lines? Can you please document them in user guide? Should we also create a more user friendly alias for clang driver?

There should not be any definition in the command-lines as it stands right now, but -fsycl should automatically add -DSYCL_INSTRUMENTATION_METADATA to provide the opportunity to disable the default argument in the future

keryell

Amazing work and useful feature!

A few remarks...
This is the first part of my review. Probably more tomorrow...

xpti/README.md

xpti/doc/SYCL_Tracing_Implementation.md

xpti/include/xpti_data_types.h

keryell

Thanks for the changes.
A few remarks for today.

sycl/include/CL/sycl/detail/cg.hpp

sycl/source/detail/scheduler/commands.cpp

xpti/README.md

keryell · 2020-02-20T01:52:21Z

I made another code review with 84 items but when I submit the review GitHub fails with:

This page is taking way too long to load.
Sorry about that. Please try refreshing and contact us if the problem persists.

keryell

Thanks for the changes.
Here are my comments for today.

keryell · 2020-02-20T02:09:46Z

Ah, GitHub is back and running...

keryell · 2020-02-20T02:11:01Z

Please avoid force-pushing, it is difficult to trace the evolution...

sycl/source/detail/event_impl.cpp

tovinkere · 2020-02-20T08:17:16Z

Thanks for the changes.
Here are my comments for today.

Thank you for the exhaustive review. All cosmetic changes have been applied along with the {} initializer. API changes to reduce type casting will be batched together for separate commit.

romanovvlad

First review.

sycl/include/CL/sycl/detail/cg.hpp

sycl/include/CL/sycl/detail/common.hpp

sycl/include/CL/sycl/detail/queue_impl.hpp

sycl/include/CL/sycl/detail/scheduler/commands.hpp

sycl/include/CL/sycl/detail/common.hpp

sycl/include/CL/sycl/detail/scheduler/commands.hpp

sycl/include/CL/sycl/ordered_queue.hpp

sycl/include/CL/sycl/queue.hpp

sycl/test/scheduler/BlockedCommands.cpp

sycl/include/CL/sycl/detail/cg.hpp

sycl/include/CL/sycl/detail/common.hpp

sycl/include/CL/sycl/detail/scheduler/commands.hpp

sycl/include/CL/sycl/handler.hpp

romanovvlad · 2020-02-21T12:27:35Z

sycl/include/CL/sycl/handler.hpp

+  // Save streams associated with this handler. Streams are then forwarded to
+  // command group and flushed in the scheduler.
+  void addStream(std::shared_ptr<detail::stream_impl> s) {
+    MStreamStorage.push_back(std::move(s));


Hm, @keryell Could you please point to some docs that say that "s" will be rvalue here?

sycl/include/CL/sycl/handler.hpp

That should have been "request changes"

sycl/include/CL/sycl/queue.hpp

sycl/source/CMakeLists.txt

sycl/source/detail/event_impl.cpp

sycl/source/detail/queue_impl.cpp

sycl/include/CL/sycl/handler.hpp

sycl/source/detail/event_impl.cpp

sycl/source/detail/pi.cpp

sycl/source/detail/scheduler/commands.cpp

sycl/CMakeLists.txt

sycl/source/CMakeLists.txt

romanovvlad · 2020-02-25T19:59:39Z

"Resolved" some conversation which seems to be resolved. @keryell please, reopen if I closed something by mistake.
@tovinkere There are several not resolved old comment. Could you please address them as well?

sycl/include/CL/sycl/detail/cg.hpp

keryell

Interesting feature!

After having a better view of the project, I think that you tried to adapt the Windows API using some std::wstring to the POSIX world but it seems simpler and more efficient to do the opposite and keep the Windows idiosyncrasies under very local control instead. For some discussion: https://stackoverflow.com/questions/402283/stdwstring-vs-stdstring

sycl/source/detail/scheduler/commands.cpp

xpti/README.md

xpti/include/xpti_data_types.h

xpti/src/xpti_proxy.cpp

xpti/include/xpti_trace_framework.hpp

xpti/src/xpti_proxy.cpp

tovinkere · 2020-02-26T05:14:49Z

Interesting feature!

After having a better view of the project, I think that you tried to adapt the Windows API using some std::wstring to the POSIX world but it seems simpler and more efficient to do the opposite and keep the Windows idiosyncrasies under very local control instead. For some discussion: https://stackoverflow.com/questions/402283/stdwstring-vs-stdstring

@keryell The std::wstring is primarily used to store wide string paths in the case of Windows with localization. To keep the interface simpe for this, std::wstring was applied to both WIndows and Linux/macOS

keryell

Great improvement!
The best code we can write is the one we do not write, because there is no bug in it. :-)

xpti/include/xpti_trace_framework.hpp

xpti/include/xpti_data_types.h

keryell

Almost done!

xpti/include/xpti_trace_framework.h

xpti/include/xpti_trace_framework.hpp

sycl/CMakeLists.txt

sycl/source/CMakeLists.txt

sycl/source/detail/event_impl.cpp

sycl/source/detail/scheduler/commands.cpp

xpti/doc/SYCL_Tracing_Implementation.md

sycl/CMakeLists.txt

sycl/source/CMakeLists.txt

sycl/source/detail/scheduler/commands.cpp

xpti/include/xpti_trace_framework.hpp

xpti/include/xpti_data_types.h

romanovvlad

LGTM

keryell

That looks good.

romanovvlad · 2020-03-07T11:57:18Z

@tovinkere Please, fix build issue reported by CI.

sycl/CMakeLists.txt

sycl/source/CMakeLists.txt

romanovvlad · 2020-03-10T07:40:07Z

xpti/src/CMakeLists.txt

-    target_compile_options(xpti PRIVATE ${XPTI_CXX_FLAGS_RELEASE})
-  endif()
+  add_library(xptid STATIC ${SOURCES})
+  target_compile_options(xptid PRIVATE ${XPTI_CXX_FLAGS_DEBUG})


I believe the correct way to form options for xptid is to do the same things which are done in sycl/source/CMakeLists.txt:132-175. Please, fix in a separate PR.

romanovvlad · 2020-03-10T07:42:08Z

@tovinkere There are some commits which are not sign-off'ed. This blocks merge, please resolve.
Suggest squashing all the commits.

+ XPTI proxy library to provide entry points into the xpti instrumentation framework + Header specification of the XPTI framework API calls + Documentation describing the changes to SYCL runtime available under xpti/doc + SYCL runtime instrumentation using XPTI proxy library to monitor the creation of the asynchronous task graph (nodes and dependencies) + Instrumentation of entry points in queue to capture the end-user source code locations of calls to submit, parallel_for etc. + Updates to the CMakeLists.txt to soft enable the XPTI instrumentation and linking of the SYCL library with the XPTI proxy/stub library + Updates to the CI scripts to include XPTI proxy library in building along with enabling of the instrumentation in the SYCL library Signed-off-by: Vasanth Tovinkere <vasanth.tovinkere@intel.com>

…e_api_test * origin/sycl: [SYCL][NFC] Fix static code analysis concerns (intel#1283) [SYCL] Fix the test/basic_tests/buffer/subbuffer.cpp (intel#1277) [SYCL][CUDA] Implement the program kernel names query (intel#1248) [SYCL] Honor the LLVM_LIBDIR_SUFFIX variable at installation time (intel#1261) [SYCL][UX] Diagnostic for undefined device functions (intel#1026) [SYCL] Reverse reqd_work_group_size attribute (intel#1234) [SYCL] Rename project to oneAPI DPC++ Compiler (intel#1249) [SYCL][XPTI] Instrumentation of SYCL runtime with XPTI (intel#1129)

…st_commit * otcshare/sycl: (469 commits) [SYCL] Implement thread-local storage restriction (intel#1281) [Driver][SYCL][FPGA] Adjust the output location for the project report (intel#1278) [SYCL][NFC] Fix static code analysis concerns (intel#1283) [SYCL] Fix the test/basic_tests/buffer/subbuffer.cpp (intel#1277) [SYCL][CUDA] Implement the program kernel names query (intel#1248) [SYCL] Honor the LLVM_LIBDIR_SUFFIX variable at installation time (intel#1261) [SYCL][UX] Diagnostic for undefined device functions (intel#1026) [SYCL] Reverse reqd_work_group_size attribute (intel#1234) [SYCL] Rename project to oneAPI DPC++ Compiler (intel#1249) [SYCL][XPTI] Instrumentation of SYCL runtime with XPTI (intel#1129) [SYCL] Add buffer dimensions restriction (intel#1147) [SYCL][NFC] Update copyright header in handler files (intel#1271) [SYCL][NFC] Format the code with clang-format [SYCL][Test] Fix SYCL library location path for LIT tests (intel#1228) [SYCL][NFC] Fix doxygen warnings (intel#1270) [SYCL][CUDA] Add the CUDA backend to the deploy-sycl-toolchain target (intel#1268) [SYCL][NFC] Fix a misleading comment regarding the SYCL flow (intel#1266) Change capability for SpecId decoration README.md: Mention retrieving llvm archive signatures travis: Restore macOS builds ...

…_accessor_refactor * origin/sycl: (454 commits) [SYCL][NFC] Fix static code analysis concerns (intel#1283) [SYCL] Fix the test/basic_tests/buffer/subbuffer.cpp (intel#1277) [SYCL][CUDA] Implement the program kernel names query (intel#1248) [SYCL] Honor the LLVM_LIBDIR_SUFFIX variable at installation time (intel#1261) [SYCL][UX] Diagnostic for undefined device functions (intel#1026) [SYCL] Reverse reqd_work_group_size attribute (intel#1234) [SYCL] Rename project to oneAPI DPC++ Compiler (intel#1249) [SYCL][XPTI] Instrumentation of SYCL runtime with XPTI (intel#1129) [SYCL] Add buffer dimensions restriction (intel#1147) [SYCL][NFC] Update copyright header in handler files (intel#1271) [SYCL][NFC] Format the code with clang-format [SYCL][Test] Fix SYCL library location path for LIT tests (intel#1228) [SYCL][NFC] Fix doxygen warnings (intel#1270) [SYCL][CUDA] Add the CUDA backend to the deploy-sycl-toolchain target (intel#1268) Change capability for SpecId decoration README.md: Mention retrieving llvm archive signatures travis: Restore macOS builds Fix DebugInfo creation after LLVM change 7a42bab Add more missing mixes Add missing fixes ...

…ntal status. (intel#1129) Tests changes for intel#6524 Signed-off-by: Rajiv Deodhar <rajiv.deodhar@intel.com> Co-authored-by: JackAKirk <jack.kirk@codeplay.com>

tovinkere requested review from bader and romanovvlad February 13, 2020 19:08

tovinkere force-pushed the xpti_instrumentation branch from 855fa3e to e622a4d Compare February 15, 2020 00:34

AGindinson requested a review from alexbatashev February 17, 2020 09:06

alexbatashev reviewed Feb 18, 2020

View reviewed changes

sycl/include/CL/sycl/detail/cg.hpp Outdated Show resolved Hide resolved

bader changed the title ~~SYCL Instrumentation for application profiling~~ [SYCL] Instrumentation for application profiling Feb 18, 2020

bader assigned romanovvlad Feb 18, 2020

keryell suggested changes Feb 19, 2020

View reviewed changes

tovinkere force-pushed the xpti_instrumentation branch from 95cd2bc to 9fef108 Compare February 19, 2020 18:32

keryell suggested changes Feb 20, 2020

View reviewed changes

alexbatashev reviewed Feb 20, 2020

View reviewed changes

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

romanovvlad requested a review from alexbatashev February 20, 2020 14:50

romanovvlad requested changes Feb 20, 2020

View reviewed changes

alexbatashev suggested changes Feb 20, 2020

View reviewed changes

romanovvlad requested changes Feb 21, 2020

View reviewed changes

alexbatashev previously approved these changes Feb 21, 2020

View reviewed changes

sycl/include/CL/sycl/handler.hpp Outdated Show resolved Hide resolved

alexbatashev self-requested a review February 21, 2020 12:46

romanovvlad requested changes Feb 21, 2020

View reviewed changes

romanovvlad requested changes Feb 25, 2020

View reviewed changes

bader reviewed Feb 25, 2020

View reviewed changes

sycl/include/CL/sycl/detail/cg.hpp Outdated Show resolved Hide resolved

keryell suggested changes Feb 26, 2020

View reviewed changes

keryell suggested changes Feb 27, 2020

View reviewed changes

keryell reviewed Feb 28, 2020

View reviewed changes

xpti/include/xpti_data_types.h Outdated Show resolved Hide resolved

xpti/include/xpti_data_types.h Outdated Show resolved Hide resolved

keryell suggested changes Feb 28, 2020

View reviewed changes

xpti/include/xpti_trace_framework.h Outdated Show resolved Hide resolved

xpti/include/xpti_trace_framework.hpp Outdated Show resolved Hide resolved

xpti/include/xpti_trace_framework.hpp Outdated Show resolved Hide resolved

xpti/include/xpti_trace_framework.hpp Outdated Show resolved Hide resolved

romanovvlad reviewed Mar 2, 2020

View reviewed changes

romanovvlad requested changes Mar 2, 2020

View reviewed changes

alexbatashev reviewed Mar 3, 2020

View reviewed changes

sycl/CMakeLists.txt Outdated Show resolved Hide resolved

sycl/source/CMakeLists.txt Outdated Show resolved Hide resolved

sycl/source/CMakeLists.txt Outdated Show resolved Hide resolved

romanovvlad reviewed Mar 3, 2020

View reviewed changes

sycl/source/detail/scheduler/commands.cpp Outdated Show resolved Hide resolved

keryell suggested changes Mar 3, 2020

View reviewed changes

xpti/include/xpti_trace_framework.hpp Outdated Show resolved Hide resolved

xpti/include/xpti_trace_framework.hpp Outdated Show resolved Hide resolved

xpti/include/xpti_trace_framework.hpp Show resolved Hide resolved

keryell suggested changes Mar 4, 2020

View reviewed changes

xpti/include/xpti_data_types.h Outdated Show resolved Hide resolved

xpti/include/xpti_data_types.h Outdated Show resolved Hide resolved

romanovvlad reviewed Mar 4, 2020

View reviewed changes

keryell previously approved these changes Mar 4, 2020

View reviewed changes

tovinkere dismissed keryell’s stale review via c3b0436 March 4, 2020 22:46

romanovvlad reviewed Mar 9, 2020

View reviewed changes

sycl/CMakeLists.txt Outdated Show resolved Hide resolved

romanovvlad reviewed Mar 9, 2020

View reviewed changes

sycl/source/CMakeLists.txt Outdated Show resolved Hide resolved

romanovvlad reviewed Mar 10, 2020

View reviewed changes

romanovvlad previously approved these changes Mar 10, 2020

View reviewed changes

tovinkere dismissed romanovvlad’s stale review via 8d7eed9 March 10, 2020 14:35

tovinkere force-pushed the xpti_instrumentation branch from fdd15ab to 8d7eed9 Compare March 10, 2020 14:35

romanovvlad approved these changes Mar 10, 2020

View reviewed changes

romanovvlad merged commit 9bf81eb into intel:sycl Mar 10, 2020

tovinkere deleted the xpti_instrumentation branch March 10, 2020 18:05

bader mentioned this pull request Jul 26, 2022

[SYCL][ABI-Break] Re-design the handling of optional code location argument for USM calls and honor memory allocation properties #6474

Merged

[SYCL] Instrumentation for application profiling #1129

[SYCL] Instrumentation for application profiling #1129

Uh oh!

Conversation

tovinkere commented Feb 13, 2020

Overview

Uh oh!

alexbatashev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tovinkere commented Feb 18, 2020

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keryell commented Feb 20, 2020

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

keryell commented Feb 20, 2020

Uh oh!

keryell commented Feb 20, 2020

Uh oh!

Uh oh!

tovinkere commented Feb 20, 2020

Uh oh!

romanovvlad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!