[SYCL] Add ITT annotation instructions #3299

MrSidims · 2021-03-03T20:28:44Z

This patch introduces InstrumentalAnnotationsPass pass that
adds ITT instrumentations for barrier and barrier-like functions
and atomic instructions. The pass is being included to
compiler flow only when "-fsycl-instrument-device-code" option
passed to clang driver.

Following annotations were added:

__itt_offload_wi_start_wrapper
Notify tools work-item execution has started
__itt_offload_wi_resume_wrapper
Notify tools work-item execution resumed (e.g. after barrier)
__itt_offload_wi_finish_wrapper
Notify tools work-item execution has finished
__itt_offload_wg_barrier
Notify tools work-item has reached a barrier
__itt_offload_atomic_op_start
Atomic operation markup
__itt_offload_atomic_op_finish
Atomic operation markup

Signed-off-by: Dmitry Sidorov [email protected]

This patch introduces InstrumentalAnnotationsPass pass to sycl-post-link tool. The pass is being included to sycl-post-link chain only when '-fsycl_device_code_add_instrumentation_calls' option is passed to clang driver. Current version of the pass create instruction to notify profiling tools that: work item has started/finised or resumed (in case if barrier was called) and it annotates the barrier call itself and for annotation of atomic instructions. Signed-off-by: Dmitry Sidorov <[email protected]>

Signed-off-by: Dmitry Sidorov <[email protected]>

MrSidims · 2021-03-03T20:32:31Z

Resolve TODO https://github.com/intel/llvm/pull/3299/files#diff-79ddf74d80f0639bcdf2d93f0bbf8aa1bd46f18c2652d0fcfc092dac7d385018R146
Add more tests?

emitCall function will be moved to a separate header in a separate patch

Signed-off-by: Dmitry Sidorov <[email protected]>

llvm/test/tools/sycl-post-link/itt_atomic_load.ll

llvm/test/tools/sycl-post-link/itt_barrier.ll

AlexeySachkov · 2021-03-03T20:50:43Z

llvm/tools/sycl-post-link/InstrumentalAnnotations.cpp

+  else if (AtomicName.contains(SPIRV_ATOMIC_STORE))
+    AtomicOp = ConstantInt::get(Int32Ty, 1);
+  else
+    AtomicOp = ConstantInt::get(Int32Ty, 2);


I suggest to add an assertion that name contains something like exchange or compare_exchange here

AlexeySachkov · 2021-03-03T20:51:43Z

llvm/tools/sycl-post-link/InstrumentalAnnotations.cpp

+  // TODO: Third parameter of Atomic Start/Finish annotation is an ordering
+  // semanticof the instruction, encoded into a value of enum, defined like this
+  // on user's/profiler's side:
+  // enum __itt_atomic_mem_order_t
+  // {
+  //   __itt_mem_order_relaxed = 0,
+  //   __itt_mem_order_acquire = 1,
+  //   __itt_mem_order_release = 2
+  // }
+  // which isn't 1:1 mapped on SPIR-V memory ordering mask, need to align it.


Tagging @vzakhari and @MichaelTutin here for awareness

It seems that combination of AcquireRelease semantic is missing in itt annotations. SequentiallyConsistent and release-consume semantics (if SPIR-V has some form of it) are not required for analysis at the moment.

I think we can either change this enum to a set of flags (acquire, release) or to add one more value to the enum. Another option would be to pass SPIR-V or C++ ordering value as is, but that will require extra argument to pass API name and tool will need to extract required details in runtime (a bit of extra overhead).
What is your opinion?

My vote goes to

to add one more value to the enum.

with that done, I can parse SPIR-V Memory Semantic mask to enum value without semantic changes.

Ok. I've updated the document and ITT header file. The enum was changed to:

enum __itt_atomic_mem_order_t { __itt_mem_order_relaxed = 0, __itt_mem_order_acquire = 1, __itt_mem_order_release = 2, __itt_mem_order_acquire_release = 3 }

llvm/tools/sycl-post-link/InstrumentalAnnotations.cpp

kbobrovs

Please address this question first before getting to my other comments. From the first glance I don't see why this should be part of sycl-post-link.

Is there a reason why this transformation needs to be done on linked code and can't be done on object code? Please state it here. Otherwise the transformation would have to go to BackendUtils.cpp or elsewhere.

clang/include/clang/Driver/Options.td

llvm/test/tools/sycl-post-link/itt_atomic_load.ll

llvm/tools/sycl-post-link/InstrumentalAnnotations.cpp

llvm/tools/sycl-post-link/sycl-post-link.cpp

Signed-off-by: Dmitry Sidorov <[email protected]>

keryell · 2021-03-06T07:34:55Z

Not clear what it is about. After some Googling, this seems to be related to Intel VTune: https://software.intel.com/content/www/us/en/develop/documentation/vtune-help/top/api-support/instrumentation-and-tracing-technology-apis/basic-usage-and-configuration/minimizing-itt-api-overhead.html
Is it something already used in Clang/LLVM upstream?
Otherwise some documentation is always appreciated. :-)
For example to add other markers, or to recycle the infrastructure for other instrumentation frameworks.

bader · 2021-03-07T07:05:29Z

Not clear what it is about. After some Googling, this seems to be related to Intel VTune: https://software.intel.com/content/www/us/en/develop/documentation/vtune-help/top/api-support/instrumentation-and-tracing-technology-apis/basic-usage-and-configuration/minimizing-itt-api-overhead.html
Is it something already used in Clang/LLVM upstream?
Otherwise some documentation is always appreciated. :-)
For example to add other markers, or to recycle the infrastructure for other instrumentation frameworks.

+1. Could folks working on this feature document the design in our internal documentation first, please? It's hard to follow changes in #3299 and #3279.

Signed-off-by: Dmitry Sidorov <[email protected]>

MrSidims · 2021-03-11T17:20:32Z

Push new patch set to unblock review. This comment is yet unaddressed: #3299 (comment) . Also there is an issue with passing of CodeGen option from the driver to BackendUtils.

bader · 2021-03-11T17:22:28Z

Push new patch set to unblock review. This comment is yet unaddressed: #3299 (comment) . Also there is an issue with passing of CodeGen option from the driver to BackendUtils.

And there are merge conflicts with the sycl branch.

clang/lib/CodeGen/BackendUtil.cpp

llvm/include/llvm/LinkAllPasses.h

llvm/tools/sycl-post-link/sycl-post-link.cpp

clang/test/Driver/sycl-instrumentation.cpp

Signed-off-by: Dmitry Sidorov <[email protected]>

…rumentalAnnotations

Signed-off-by: Dmitry Sidorov <[email protected]>

romanovvlad

LGTM

vzakhari · 2021-03-12T17:23:40Z

LGTM

llvm/include/llvm/Transforms/Instrumentation/SYCLITTAnnotations.h

llvm/lib/Transforms/Instrumentation/SYCLITTAnnotations.cpp

llvm/include/llvm/Transforms/Instrumentation/SYCLITTAnnotations.h

llvm/lib/Transforms/Instrumentation/SYCLITTAnnotations.cpp

Signed-off-by: Dmitry Sidorov <[email protected]>

bader

Please, update comments accordingly.

llvm/include/llvm/Transforms/Instrumentation/SPIRITTAnnotations.h

llvm/lib/Transforms/Instrumentation/SPIRITTAnnotations.cpp

Signed-off-by: Dmitry Sidorov <[email protected]>

bader

I'm okay with LLVM pass.

clang/lib/CodeGen/BackendUtil.cpp

clang/include/clang/Driver/Options.td

clang/test/CodeGenSYCL/kernel-simple-instrumentation.cpp

Signed-off-by: Dmitry Sidorov <[email protected]>

elizabethandrews

LGTM. Thanks!

MrSidims · 2021-03-16T11:48:25Z

@bader can we consider the review to be done, as we have 3 LGTMs from the appropriate code owners and 3 from Vlad, Slava and Konst?

* 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) ...

AlexeySachkov

Some post-commit comments: mostly minor ones, but the one about non-constant memory semantic might be important

llvm/lib/Transforms/Instrumentation/SPIRITTAnnotations.cpp

AlexeySachkov · 2021-03-23T17:09:22Z

llvm/lib/Transforms/Instrumentation/SPIRITTAnnotations.cpp

+  if (AtomicName.contains(SPIRV_ATOMIC_LOAD))
+    AtomicOp = ConstantInt::get(Int32Ty, 0);
+  else if (AtomicName.contains(SPIRV_ATOMIC_STORE))
+    AtomicOp = ConstantInt::get(Int32Ty, 1);
+  else
+    AtomicOp = ConstantInt::get(Int32Ty, 2);


If you change your prefixes, you could use llvm::StringSwitch here: I mean use _Z[number]__spirv_AtomicLoad and llvm::StringSwitch::StartsWith

AlexeySachkov · 2021-03-23T17:11:24Z

llvm/lib/Transforms/Instrumentation/SPIRITTAnnotations.cpp

+  uint64_t MemFlag = dyn_cast<ConstantInt>(AtomicFun->getArgOperand(2))
+                         ->getValue()
+                         .getZExtValue();


Please note that in general this is not required to be a constant value.

Also, you are using dyn_cast, but don't check if it returned nullptr

MrSidims requested review from AGindinson, AlexeySachkov, kbobrovs and mdtoguchi as code owners March 3, 2021 20:28

Test fix

573670b

Signed-off-by: Dmitry Sidorov <[email protected]>

MrSidims requested a review from vzakhari March 3, 2021 20:32

Comments fixes

0f17bac

Signed-off-by: Dmitry Sidorov <[email protected]>

AlexeySachkov reviewed Mar 3, 2021

View reviewed changes

vzakhari reviewed Mar 3, 2021

View reviewed changes

llvm/tools/sycl-post-link/InstrumentalAnnotations.cpp Outdated Show resolved Hide resolved

llvm/tools/sycl-post-link/InstrumentalAnnotations.cpp Outdated Show resolved Hide resolved

kbobrovs reviewed Mar 4, 2021

View reviewed changes

vzakhari mentioned this pull request Mar 4, 2021

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

Merged

Apply several comments

4d01c8b

Signed-off-by: Dmitry Sidorov <[email protected]>

MrSidims added 4 commits March 10, 2021 15:30

Move the pass from sycl-post-link to FE

d583984

Signed-off-by: Dmitry Sidorov <[email protected]>

Rename

1014db0

Signed-off-by: Dmitry Sidorov <[email protected]>

Change driver part

e1d768a

Signed-off-by: Dmitry Sidorov <[email protected]>

Few bug fixes

825f58a

Signed-off-by: Dmitry Sidorov <[email protected]>

MrSidims requested review from bader, elizabethandrews and premanandrao as code owners March 11, 2021 17:18

MrSidims commented Mar 11, 2021

View reviewed changes

clang/lib/CodeGen/BackendUtil.cpp Outdated Show resolved Hide resolved

MrSidims requested a review from kbobrovs March 11, 2021 17:54

kbobrovs suggested changes Mar 11, 2021

View reviewed changes

MrSidims added 2 commits March 11, 2021 22:56

Small fixes

c0809fe

Signed-off-by: Dmitry Sidorov <[email protected]>

Merge remote-tracking branch 'origin/sycl' into private/MrSidims/Inst…

f165273

…rumentalAnnotations

MrSidims added 2 commits March 12, 2021 11:09

Several fixes

8bb5cdf

Signed-off-by: Dmitry Sidorov <[email protected]>

Add diag + clang format

33e6101

Signed-off-by: Dmitry Sidorov <[email protected]>

MrSidims requested a review from kbobrovs March 12, 2021 12:04

Fix test

d525d3c

Signed-off-by: Dmitry Sidorov <[email protected]>

MrSidims requested review from vzakhari and romanovvlad March 12, 2021 13:00

Update pass header

6ab9c02

Signed-off-by: Dmitry Sidorov <[email protected]>

romanovvlad reviewed Mar 12, 2021

View reviewed changes

bader reviewed Mar 15, 2021

View reviewed changes

SYCLITT -> SPIRITT

457b1ec

Signed-off-by: Dmitry Sidorov <[email protected]>

MrSidims requested a review from bader March 15, 2021 15:51

bader reviewed Mar 15, 2021

View reviewed changes

Apply suggestions

e1aa264

Signed-off-by: Dmitry Sidorov <[email protected]>

MrSidims requested a review from bader March 15, 2021 17:55

bader previously approved these changes Mar 15, 2021

View reviewed changes

mdtoguchi previously approved these changes Mar 15, 2021

View reviewed changes

kbobrovs previously approved these changes Mar 15, 2021

View reviewed changes

elizabethandrews reviewed Mar 15, 2021

View reviewed changes

clang/lib/CodeGen/BackendUtil.cpp Outdated Show resolved Hide resolved

clang/include/clang/Driver/Options.td Outdated Show resolved Hide resolved

clang/test/CodeGenSYCL/kernel-simple-instrumentation.cpp Show resolved Hide resolved

Apply driver suggestions

a41f48a

Signed-off-by: Dmitry Sidorov <[email protected]>

MrSidims dismissed stale reviews from kbobrovs, mdtoguchi, and bader via a41f48a March 15, 2021 20:35

MrSidims requested a review from elizabethandrews March 15, 2021 20:38

elizabethandrews approved these changes Mar 16, 2021

View reviewed changes

bader merged commit be18b1d into intel:sycl Mar 16, 2021

AlexeySachkov reviewed Mar 23, 2021

View reviewed changes

MrSidims mentioned this pull request May 4, 2021

[NFC][SYCL] Apply post-commit comments to ITT annotation support #3691

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Add ITT annotation instructions #3299

[SYCL] Add ITT annotation instructions #3299

MrSidims commented Mar 3, 2021 •

edited

Loading

MrSidims commented Mar 3, 2021 •

edited

Loading

AlexeySachkov Mar 3, 2021

AlexeySachkov Mar 3, 2021

MichaelTutin Mar 4, 2021

MrSidims Mar 4, 2021 •

edited

Loading

MichaelTutin Mar 9, 2021 •

edited

Loading

kbobrovs left a comment

keryell commented Mar 6, 2021

bader commented Mar 7, 2021

MrSidims commented Mar 11, 2021

bader commented Mar 11, 2021

romanovvlad left a comment

vzakhari commented Mar 12, 2021

bader left a comment

bader left a comment

elizabethandrews left a comment

MrSidims commented Mar 16, 2021

AlexeySachkov left a comment

AlexeySachkov Mar 23, 2021

AlexeySachkov Mar 23, 2021

[SYCL] Add ITT annotation instructions #3299

[SYCL] Add ITT annotation instructions #3299

Conversation

MrSidims commented Mar 3, 2021 • edited Loading

MrSidims commented Mar 3, 2021 • edited Loading

AlexeySachkov Mar 3, 2021

Choose a reason for hiding this comment

AlexeySachkov Mar 3, 2021

Choose a reason for hiding this comment

MichaelTutin Mar 4, 2021

Choose a reason for hiding this comment

MrSidims Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

MichaelTutin Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

kbobrovs left a comment

Choose a reason for hiding this comment

keryell commented Mar 6, 2021

bader commented Mar 7, 2021

MrSidims commented Mar 11, 2021

bader commented Mar 11, 2021

romanovvlad left a comment

Choose a reason for hiding this comment

vzakhari commented Mar 12, 2021

bader left a comment

Choose a reason for hiding this comment

bader left a comment

Choose a reason for hiding this comment

elizabethandrews left a comment

Choose a reason for hiding this comment

MrSidims commented Mar 16, 2021

AlexeySachkov left a comment

Choose a reason for hiding this comment

AlexeySachkov Mar 23, 2021

Choose a reason for hiding this comment

AlexeySachkov Mar 23, 2021

Choose a reason for hiding this comment

MrSidims commented Mar 3, 2021 •

edited

Loading

MrSidims commented Mar 3, 2021 •

edited

Loading

MrSidims Mar 4, 2021 •

edited

Loading

MichaelTutin Mar 9, 2021 •

edited

Loading