[SYCL] Instrumentation for application profiling #1129
Conversation
tovinkere
force-pushed
the
xpti_instrumentation
branch
from
855fa3e to
e622a4d
Compare
alexbatashev
left a comment
alexbatashev
left a comment
There was a problem hiding this comment.
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 |
bader
changed the title
keryell
left a comment
keryell
left a comment
There was a problem hiding this comment.
Amazing work and useful feature!
A few remarks...
This is the first part of my review. Probably more tomorrow...
tovinkere
force-pushed
the
xpti_instrumentation
branch
from
95cd2bc to
9fef108
Compare
keryell
left a comment
keryell
left a comment
There was a problem hiding this comment.
Thanks for the changes.
A few remarks for today.
|
I made another code review with 84 items but when I submit the review GitHub fails with:
|
keryell
left a comment
keryell
left a comment
There was a problem hiding this comment.
Thanks for the changes.
Here are my comments for today.
|
Ah, GitHub is back and running... |
|
Please avoid force-pushing, it is difficult to trace the evolution... |
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. |
sycl/include/CL/sycl/handler.hpp
Outdated
There was a problem hiding this comment.
Hm, @keryell Could you please point to some docs that say that "s" will be rvalue here?
alexbatashev
dismissed
their stale review
That should have been "request changes"
|
"Resolved" some conversation which seems to be resolved. @keryell please, reopen if I closed something by mistake. |
keryell
left a comment
keryell
left a comment
There was a problem hiding this comment.
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
left a comment
keryell
left a comment
There was a problem hiding this comment.
Great improvement!
The best code we can write is the one we do not write, because there is no bug in it. :-)
|
@tovinkere Please, fix build issue reported by CI. |
xpti/src/CMakeLists.txt
Outdated
There was a problem hiding this comment.
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.
|
@tovinkere There are some commits which are not sign-off'ed. This blocks merge, please resolve. |
+ 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 <[email protected]>
tovinkere
force-pushed
the
xpti_instrumentation
branch
from
fdd15ab to
8d7eed9
Compare
6 participants
Overview
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.llvm/xpti/doc.