Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCL][Doc] Update docs to reflect PI removal. #15057

Open
wants to merge 16 commits into
base: sycl
Choose a base branch
from

Conversation

aarongreig
Copy link
Contributor

@aarongreig aarongreig commented Aug 13, 2024

Fixes #14928

@aarongreig aarongreig marked this pull request as ready for review August 14, 2024 09:44
@aarongreig aarongreig requested review from a team as code owners August 14, 2024 09:44
Copy link
Contributor

@dm-vodopyanov dm-vodopyanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also these doxygen docs have to be updated:

1

/// Construct a context_impl using plug-in interoperability handle.
///
/// The constructed context_impl will use the AsyncHandler parameter to
/// handle exceptions.
///
/// \param UrContext is an instance of a valid plug-in context handle.
/// \param AsyncHandler is an instance of async_handler.
/// \param Plugin is the reference to the underlying Plugin that this
/// \param OwnedByRuntime is the flag if ownership is kept by user or
/// transferred to runtime

2

/// Gets the underlying context object (if any) without reference count
/// modification.
///
/// Caller must ensure the returned object lives on stack only. It can also
/// be safely passed to the underlying native runtime API. Warning. Returned
/// reference will be invalid if context_impl was destroyed.
///
/// \return an instance of raw plug-in context handle.

3

/// Gets the underlying context object (if any) without reference count
/// modification.
///
/// Caller must ensure the returned object lives on stack only. It can also
/// be safely passed to the underlying native runtime API. Warning. Returned
/// reference will be invalid if context_impl was destroyed.
///
/// \return an instance of raw plug-in context handle.

4

/// Constructs an event instance from a plug-in event handle.
///
/// The SyclContext must match the plug-in context associated with the
/// ClEvent.
///
/// \param Event is a valid instance of plug-in event.
/// \param SyclContext is an instance of SYCL context.

5

/// Returns raw interoperability event handle. Returned reference will be
/// invalid if event_impl was destroyed.
///
/// \return a reference to an instance of plug-in event handle.
ur_event_handle_t &getHandleRef();
/// Returns raw interoperability event handle. Returned reference will be
/// invalid if event_impl was destroyed.
///
/// \return a const reference to an instance of plug-in event handle.

6

/// Constructs a SYCL kernel instance from a UrKernel
///
/// This constructor is used for plug-in interoperability. It always marks
/// kernel as being created from source.
///
/// \param Kernel is a valid UrKernel instance
/// \param Context is a valid SYCL context
/// \param KernelBundleImpl is a valid instance of kernel_bundle_impl

7

/// Constructs platform_impl from a plug-in interoperability platform
/// handle.
///
/// \param APlatform is a raw plug-in platform handle.
/// \param APlugin is a plug-in handle.

8

/// Returns raw underlying plug-in platform handle.
///
/// Unlike get() method, this method does not retain handler. It is caller
/// responsibility to make sure that platform stays alive while raw handle
/// is in use.
///
/// \return a raw plug-in platform handle.

sycl/doc/design/images/RuntimeArchitecture-with-fusion.svg Outdated Show resolved Hide resolved
sycl/doc/design/images/RuntimeArchitecture.svg Outdated Show resolved Hide resolved
@dm-vodopyanov
Copy link
Contributor

Also other things:

9

sycl/pi_win_proxy_loader @intel/llvm-reviewers-runtime

has to be updated, we don't have sycl/pi_win_proxy_loader anymore.

10

llvm/.github/CODEOWNERS

Lines 125 to 131 in 07bf3c1

llvm/**/*SYCLNativeCPU* @intel/dpcpp-nativecpu-pi-reviewers
clang/include/clang/Basic/SYCLNativeCPUHelpers.h @intel/dpcpp-nativecpu-pi-reviewers
clang/test/CodeGenSYCL/native_cpu*.cpp @intel/dpcpp-nativecpu-pi-reviewers
clang/test/Driver/sycl-native-cpu*.cpp @intel/dpcpp-nativecpu-pi-reviewers
sycl/**/native_cpu/ @intel/dpcpp-nativecpu-pi-reviewers
sycl/doc/design/SYCLNativeCPU.md @intel/dpcpp-nativecpu-pi-reviewers
sycl/include/sycl/detail/native_cpu.hpp @intel/dpcpp-nativecpu-pi-reviewers

We need to rename @intel/dpcpp-nativecpu-pi-reviewers as well (to @intel/dpcpp-nativecpu-ur-reviewers ?) @stdale-intel, can you please rename the group? @aarongreig or @stdale-intel, can you please update the CODEOWNERS file after that?

@dm-vodopyanov
Copy link
Contributor

+ more

11
https://github.com/intel/llvm/tree/sycl/sycl/test-e2e/Plugin folder has to be renamed? (+ rename in CODEOWNERS after that)

12

--cmake-opt="-DSYCL_PI_TESTS=OFF" \

13

--cmake-opt="-DSYCL_PI_TESTS=OFF"

14

sycl_build_pi_hip_platform = "AMD"

15

sycl_build_pi_hip_platform = args.hip_platform

16

"-DSYCL_BUILD_PI_HIP_PLATFORM={}".format(sycl_build_pi_hip_platform),

17

# lld is needed on Windows or for the HIP plugin on AMD

18

llvm/buildbot/configure.py

Lines 155 to 156 in 07bf3c1

if args.enable_plugin:
sycl_enabled_backends += args.enable_plugin

@dm-vodopyanov
Copy link
Contributor

dm-vodopyanov commented Aug 14, 2024

+ update XPTI

19

plugin interface (PI) layer `initialize()` call. In this call, we will
perform two operations:
1. Initialize all listeners and create a trace event to represent the graph.
This is done in `sycl/include/sycl/detail/pi.cpp`.

20

// Stream name being used for traces generated from the SYCL plugin layer

21

sycl::unittest::UrMock<> MockPlugin;

22

sycl::unittest::UrMock<> MockPlugin;

23

# Example SYCL PI Layer Collector
The SYCL PI layer collector demonstrates the creation of a subscriber and prints
of the data received from SYCL PI layer stream. In order to obtain the data from
an application instrumented with XPTI, the following steps must be performed.
1. Set the environment variable that indicates that tracing has been enabled.
This is defined by the variable `XPTI_TRACE_ENABLE`. The possible
values taken by this environment variable are:
To enable: `XPTI_TRACE_ENABLE=1` or `XPTI_TRACE_ENABLE=true`
To disable: `XPTI_TRACE_ENABLE=0` or `XPTI_TRACE_ENABLE=false`
2. Set the environment variable that points to the XPTI framework dispatcher so
the stub library can dynamically load it and dispatch the calls to the
dispatcher.
`XPTI_FRAMEWORK_DISPATCHER=/path/to/libxptifw.[so,dll,dylib]`
3. Set the environment variable that points to the subscriber, which in this
case is `libsyclpi_collector.[so,dll,dylib]`.
`XPTI_SUBSCRIBERS=/path/to/libxpti_syclpi_collector.[so,dll,dylib]`

24

// generated by the SYCL PI layer. The protocol described by the PI layer

25

XPTI_CALLBACK_API void syclPiCallback(uint16_t TraceType,

26

echo -e " Example:- ${yellow}sycl,sycl.pi,sycl.perf ${clear}${green}"

27

// Stream name being used for traces generated from the SYCL plugin layer
inline constexpr const char *SYCL_PICALL_STREAM_NAME = "sycl.pi";
// Stream name being used for traces generated from UR calls. This stream
// contains information about function arguments.
inline constexpr const char *SYCL_PIDEBUGCALL_STREAM_NAME = "sycl.pi.debug";

28

add_library(xpti_syclpi_collector SHARED
syclpi_collector.cpp
)
target_link_libraries(xpti_syclpi_collector PRIVATE
${xptifw_lib}
${CMAKE_DL_LIBS}
)
target_include_directories(xpti_syclpi_collector PRIVATE
${XPTIFW_DIR}/include
${XPTI_DIR}/include
${XPTIFW_DIR}/samples/include
)
if (XPTI_ENABLE_TBB)
target_link_libraries(xpti_syclpi_collector PRIVATE tbb)

(@KseniyaTikhomirova can provide more information about the update of XPTI (if any)).

@dm-vodopyanov
Copy link
Contributor

29

changed, e.g.: `[PI]`, `[CUDA]`, `[Doc]`.

@dm-vodopyanov
Copy link
Contributor

+

30

The backend is tested by a relevant device/toolkit prior to a ONEAPI plugin release.
Go to the plugin release
[pages](https://developer.codeplay.com/products/oneapi/nvidia/) for further
details.

31

oneAPI plugin release. Go to the plugin release
[pages](https://developer.codeplay.com/products/oneapi/amd) for further details.

@dm-vodopyanov
Copy link
Contributor

32

add_sycl_library("pi_${PLUGIN_NAME}" SHARED
LINKER_SCRIPT "${PROJECT_SOURCE_DIR}/plugins/ld-version-script.txt"
SOURCES ${ARG_SOURCES}
INCLUDE_DIRS
${ARG_INCLUDE_DIRS}
${sycl_inc_dir}
LIBRARIES
${ARG_LIBRARIES}
OpenCL-Headers
)
# All SYCL plugins use UR sources.
# Disable errors from warnings and apply other workarounds while building the UR.
if(WIN32)
target_compile_options("pi_${PLUGIN_NAME}" PRIVATE /WX- /UUNICODE /DUSE_Z7=ON)
else()
target_compile_options("pi_${PLUGIN_NAME}" PRIVATE -Wno-error)
endif()
# Install feature test header
if (NOT "${ARG_HEADER}" STREQUAL "")
get_filename_component(HEADER_NAME ${ARG_HEADER} NAME)
configure_file(
${ARG_HEADER}
${SYCL_INCLUDE_BUILD_DIR}/sycl/detail/plugins/${PLUGIN_NAME}/${HEADER_NAME}
COPYONLY)
install(FILES ${ARG_HEADER}
DESTINATION ${SYCL_INCLUDE_DIR}/sycl/detail/plugins/${PLUGIN_NAME}
COMPONENT pi_${PLUGIN_NAME})
endif()
install(TARGETS pi_${PLUGIN_NAME}
LIBRARY DESTINATION "lib${LLVM_LIBDIR_SUFFIX}" COMPONENT pi_${PLUGIN_NAME}
RUNTIME DESTINATION "bin" COMPONENT pi_${PLUGIN_NAME})
set (manifest_file
${CMAKE_CURRENT_BINARY_DIR}/install_manifest_pi_${PLUGIN_NAME}.txt)
add_custom_command(OUTPUT ${manifest_file}
COMMAND "${CMAKE_COMMAND}"
"-DCMAKE_INSTALL_COMPONENT=pi_${PLUGIN_NAME}"
-P "${CMAKE_BINARY_DIR}/cmake_install.cmake"
COMMENT "Deploying component pi_${PLUGIN_NAME}"
USES_TERMINAL
)
add_custom_target(install-sycl-plugin-${PLUGIN_NAME}
DEPENDS
${manifest_file} pi_${PLUGIN_NAME}
)
set_property(GLOBAL APPEND PROPERTY SYCL_TOOLCHAIN_INSTALL_COMPONENTS
pi_${PLUGIN_NAME})

@aarongreig aarongreig requested review from a team and bader as code owners August 16, 2024 11:00
@aarongreig
Copy link
Contributor Author

@dm-vodopyanov thanks for all the feedback

For point #1, the comment is still correct as it refers to the Plugin object (which arguably should be renamed but that isn't a task for this PR)
For points #14, #15 and #16, the cmake variable is still called that. I think I'll spin out a separate PR to rename it.
Points #30 and #31 are kind of weird, we still call them "plugin releases" on our website (the page pointed to). That's an issue I can raise internally, but it might be worth leaving the wording as is to match up with what you see if you click the link.

I'm not sure what to do about #23, #24, #25 and #26. The pi_collector example will for sure need re-written now that sycl.pi has been removed, I'll open another sub-ticket on the post-merge work issue to look into this as it's not as trivial as just updating docs.

@aarongreig
Copy link
Contributor Author

Points #30 and #31 are kind of weird, we still call them "plugin releases" on our website (the page pointed to). That's an issue I can raise internally, but it might be worth leaving the wording as is to match up with what you see if you click the link.

It isn't clear if we'll change the terminology on the website, but it for sure wont be until after the next release since the current latest release on there is inarguably a PI plugin.

@aarongreig
Copy link
Contributor Author

ping @intel/dpcpp-specification-reviewers @intel/dpcpp-devops-reviewers

Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing just the specification change ... the change to "sycl_ext_oneapi_discard_queue_events.asciidoc" is OK.

@aarongreig
Copy link
Contributor Author

@steffenlarsen would you mind taking a look at this so we can get it over the line?

@aarongreig
Copy link
Contributor Author

Also other things:
9

sycl/pi_win_proxy_loader @intel/llvm-reviewers-runtime

has to be updated, we don't have sycl/pi_win_proxy_loader anymore.
10

llvm/.github/CODEOWNERS

Lines 125 to 131 in 07bf3c1

llvm/**/*SYCLNativeCPU* @intel/dpcpp-nativecpu-pi-reviewers
clang/include/clang/Basic/SYCLNativeCPUHelpers.h @intel/dpcpp-nativecpu-pi-reviewers
clang/test/CodeGenSYCL/native_cpu*.cpp @intel/dpcpp-nativecpu-pi-reviewers
clang/test/Driver/sycl-native-cpu*.cpp @intel/dpcpp-nativecpu-pi-reviewers
sycl/**/native_cpu/ @intel/dpcpp-nativecpu-pi-reviewers
sycl/doc/design/SYCLNativeCPU.md @intel/dpcpp-nativecpu-pi-reviewers
sycl/include/sycl/detail/native_cpu.hpp @intel/dpcpp-nativecpu-pi-reviewers

We need to rename @intel/dpcpp-nativecpu-pi-reviewers as well (to @intel/dpcpp-nativecpu-ur-reviewers ?) @stdale-intel, can you please rename the group? @aarongreig or @stdale-intel, can you please update the CODEOWNERS file after that?

Adding @stdale-intel to reviewers because of the action above

maybe we can spin this out into its own issue?

@dm-vodopyanov @stdale-intel I've created an issue to track so we can unblock merging this #15956

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable! Just one small mistake. 👍

sycl/doc/design/SYCLInstrumentationUsingXPTI.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants