Skip to content

ITT stubs and wrappers for SPIR-V devices. #3279

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Mar 15, 2021
Merged

ITT stubs and wrappers for SPIR-V devices. #3279

merged 11 commits into from
Mar 15, 2021

Conversation

vzakhari
Copy link
Contributor

@vzakhari vzakhari commented Mar 2, 2021 •
edited
Loading

Added definitions of ITT related APIs into SPIR-V libdevice.
The APIs may be used in user device code, and also the compiler may use them
for automatic instrumentation of the device code.
Tools like Intel Inspector should be able to recognize the calls in the device code
by their names.

Signed-off-by: Vyacheslav Zakharin vyacheslav.p.zakharin@intel.com

ITT stubs and compiler wrappers for SPIR-V devices.
a985517 
Signed-off-by: Vyacheslav Zakharin <vyacheslav.p.zakharin@intel.com>
Vyacheslav Zakharin added 2 commits March 1, 2021 19:40
clang-format
e2e60cb 
Signed-off-by: Vyacheslav Zakharin <vyacheslav.p.zakharin@intel.com>
Removed \t.
47a8102 
Signed-off-by: Vyacheslav Zakharin <vyacheslav.p.zakharin@intel.com>
@jinge90
Copy link
Contributor

jinge90 commented Mar 2, 2021

Hi, @vzakhari
What's the intention of this patch?

@vzakhari
Copy link
Contributor Author

vzakhari commented Mar 2, 2021

Hi, @vzakhari
What's the intention of this patch?

Hi @jinge90, I will have to create a document regarding these APIs. In the meantime, I will send you some links by mail.

jinge90
jinge90 reviewed Mar 2, 2021
View reviewed changes
MichaelTutin
MichaelTutin reviewed Mar 2, 2021
View reviewed changes
Vyacheslav Zakharin added 2 commits March 3, 2021 08:21
Renamed itt_cmplr_wrappers.cpp
ea511ff 
Signed-off-by: Vyacheslav Zakharin <vyacheslav.p.zakharin@intel.com>
Cleaned up names and added user wrappers.
ab8c04c 
Signed-off-by: Vyacheslav Zakharin <vyacheslav.p.zakharin@intel.com>
@vzakhari vzakhari changed the title ITT stubs and compiler wrappers for SPIR-V devices. ITT stubs and wrappers for SPIR-V devices. Mar 3, 2021
clang-format
2d9c6c6 
Signed-off-by: Vyacheslav Zakharin <vyacheslav.p.zakharin@intel.com>
@MrSidims MrSidims mentioned this pull request Mar 4, 2021
Merged
@vzakhari
Copy link
Contributor Author

vzakhari commented Mar 5, 2021

The test fail is going to be fixed in #3311.

Revert changes that uncovered __spirv_GlobalInvocationId_x issue.
089a806 
Signed-off-by: Vyacheslav Zakharin <vyacheslav.p.zakharin@intel.com>
@romanovvlad romanovvlad requested a review from jinge90 March 8, 2021 11:18
romanovvlad
romanovvlad previously approved these changes Mar 8, 2021
View reviewed changes
Copy link
Contributor

@romanovvlad romanovvlad left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, but it would be nice if @jinge90 approved as well.

jinge90
jinge90 approved these changes Mar 9, 2021
View reviewed changes
Copy link
Contributor

@jinge90 jinge90 left a comment

Choose a reason for hiding this comment

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

LGTM.

@jinge90 jinge90 self-requested a review March 9, 2021 03:24
jinge90
jinge90 previously approved these changes Mar 9, 2021
View reviewed changes
Copy link
Contributor

@jinge90 jinge90 left a comment

Choose a reason for hiding this comment

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

LGTM.
And a minor suggestion, it seems that spirv builtin like "__spirv_BuiltInWorkgroupId" may be used in other libdevice components, what about making a wrapper function in libdevice for each builtin used in libdevice and invoked the wrapper function instead of the builtin itself? If the builtin changed the in future compiler, we don't need to modify all sources depending on them, updating the wrapper function should be OK.
Thanks very much.

@vzakhari
Copy link
Contributor Author

vzakhari commented Mar 9, 2021

LGTM.
And a minor suggestion, it seems that spirv builtin like "__spirv_BuiltInWorkgroupId" may be used in other libdevice components, what about making a wrapper function in libdevice for each builtin used in libdevice and invoked the wrapper function instead of the builtin itself? If the builtin changed the in future compiler, we don't need to modify all sources depending on them, updating the wrapper function should be OK.
Thanks very much.

I agree. This should be done as part of the FIXME note.

Added documentation.
b5b3ff1 
Signed-off-by: Vyacheslav Zakharin <vyacheslav.p.zakharin@intel.com>
@vzakhari vzakhari dismissed stale reviews from jinge90 and romanovvlad via b5b3ff1 March 9, 2021 19:47
@vzakhari vzakhari requested a review from a team as a code owner March 9, 2021 19:47
keryell
keryell suggested changes Mar 9, 2021
View reviewed changes
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Thanks for the doc.
Just some minor formatting style issue.

Fomatting fixed.
e80f1b4 
Signed-off-by: Vyacheslav Zakharin <vyacheslav.p.zakharin@intel.com>
jinge90
jinge90 previously approved these changes Mar 10, 2021
View reviewed changes
@jinge90
Copy link
Contributor

jinge90 commented Mar 10, 2021

Hi, @vzakhari
When this PR is merged, do we need to enable "stub" library right away? If so, I need some updates in current "2-step" link.
Thanks very much.

@vzakhari
Copy link
Contributor Author

Hi, @vzakhari
When this PR is merged, do we need to enable "stub" library right away? If so, I need some updates in current "2-step" link.
Thanks very much.

Hi @jinge90, we will need to link the compiler wrappers and the stubs whenever the compiler instrumentation pass is ready to be enabled. Linking the user wrappers is not urgent.

@jinge90
Copy link
Contributor

jinge90 commented Mar 10, 2021

Hi, @vzakhari
When this PR is merged, do we need to enable "stub" library right away? If so, I need some updates in current "2-step" link.
Thanks very much.

Hi @jinge90, we will need to link the compiler wrappers and the stubs whenever the compiler instrumentation pass is ready to be enabled. Linking the user wrappers is not urgent.

Hi, @vzakhari
I expect the "stub" library should be always linked but the compiler wrapper or user wrapper is not, is it correct?

@vzakhari
Copy link
Contributor Author

Hi, @vzakhari
When this PR is merged, do we need to enable "stub" library right away? If so, I need some updates in current "2-step" link.
Thanks very much.

Hi @jinge90, we will need to link the compiler wrappers and the stubs whenever the compiler instrumentation pass is ready to be enabled. Linking the user wrappers is not urgent.

Hi, @vzakhari
I expect the "stub" library should be always linked but the compiler wrapper or user wrapper is not, is it correct?

I believe we should be able to link them all always and using -only-needed. Though, we have to make sure that the stub part is linked after the user wrappers and compiler wrappers parts.

@vzakhari
Copy link
Contributor Author

Hi, @vzakhari
When this PR is merged, do we need to enable "stub" library right away? If so, I need some updates in current "2-step" link.
Thanks very much.

Hi @jinge90, we will need to link the compiler wrappers and the stubs whenever the compiler instrumentation pass is ready to be enabled. Linking the user wrappers is not urgent.

Hi, @vzakhari
I expect the "stub" library should be always linked but the compiler wrapper or user wrapper is not, is it correct?

I believe we should be able to link them all always and using -only-needed. Though, we have to make sure that the stub part is linked after the user wrappers and compiler wrappers parts.

This actually depends on #3299, so the driver has to be changed along with or after it.

MrSidims
MrSidims reviewed Mar 10, 2021
View reviewed changes
Updated __itt_atomic_mem_order_t enum.
0c091bb 
Signed-off-by: Vyacheslav Zakharin <vyacheslav.p.zakharin@intel.com>
MrSidims
MrSidims approved these changes Mar 10, 2021
View reviewed changes
Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

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

LGTM

@vzakhari
Copy link
Contributor Author

@jbrodman, @mkinsner, @jinge90, can you please review?

@romanovvlad romanovvlad merged commit dc82826 into intel:sycl Mar 15, 2021
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Mar 15, 2021
Merge remote-tracking branch 'upstream/sycl' into sycl_interop
07f3686 
* upstream/sycl:
  Add ITT stubs and wrappers for SPIR-V devices (intel#3279)
  [SYCL] Add zero argument version of buffer::reinterpret() for SYCL 2020 (intel#3333)
  [SYCL] Restore old behavior of get() method (intel#3356)
  [Driver][SYCL][FPGA] Improve FPGA AOT when using Triple (intel#3330)
  [SYCL] Revert support for pinned_host_memory extension in Level-Zero backend. Make it a NOP (intel#3349)
  [SYCL] Remove redundant build options processing (intel#3342)
  [SYCL][NFC] Factor out GenXIntrinsics git repo tag. (intel#3351)
  [SYCL] Add coarse-grained debug aid for finding imbalance in Level-Zero alloc/free (intel#3334)
  [SYCL] Sub-group load/store for raw pointers (intel#3255)
  [SYCL] Fix more unused variables warnings (intel#3352)
  [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process  (intel#3338)
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Mar 19, 2021
Merge remote-tracking branch 'upstream/sycl' into deprecate_old_apis
f1224e5 
* upstream/sycl: (1804 commits)
  [SYCL] SYCL 2020 backend interoperability part 1 (intel#3354)
  [Driver][SYCL] Address issue when unbundling for non-FPGA archive (intel#3366)
  [SYCL][Doc] Update compiler options descriptions (intel#3340)
  [SYCL] Update ABI dump tool to disable checks with libcxx by default (intel#3370)
  [SYCL] Update the way we handle -sycl-std= based on community review feedback (intel#3371)
  [SYCL] Move tests to llvm-test-suite (intel#3359)
  [SYCL][PI][L0] Revert copy batching from intel#3232 (intel#3363)
  [SYCL] Remove unsupported option.
  [SYCL] Fix pragma setting in sycl-post-link (intel#3358)
  [SYCL] Add ITT annotation instructions (intel#3299)
  [SYCL] Propagate attributes of original kernel to wrapper kernel generated for range-rounding (intel#3306)
  [BuildBot] Uplift GPU RT version for Linux to 21.09.19150 (intel#3316)
  [SYCL] Retain PI events until they have signaled (intel#3350)
  [SYCL] Add caching when using interop constructor (intel#3327)
  Add ITT stubs and wrappers for SPIR-V devices (intel#3279)
  [SYCL] Add zero argument version of buffer::reinterpret() for SYCL 2020 (intel#3333)
  [SYCL] Restore old behavior of get() method (intel#3356)
  [Driver][SYCL][FPGA] Improve FPGA AOT when using Triple (intel#3330)
  [SYCL] Revert support for pinned_host_memory extension in Level-Zero backend. Make it a NOP (intel#3349)
  [SYCL] Remove redundant build options processing (intel#3342)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader left review comments

@MichaelTutin MichaelTutin MichaelTutin left review comments

@keryell keryell keryell approved these changes

@jbrodman jbrodman jbrodman approved these changes

@MrSidims MrSidims MrSidims approved these changes

@jinge90 jinge90 jinge90 approved these changes

@romanovvlad romanovvlad romanovvlad left review comments

@AlexeySachkov AlexeySachkov Awaiting requested review from AlexeySachkov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@vzakhari @jinge90 @bader @keryell @jbrodman @romanovvlad @MrSidims @MichaelTutin