[SYCL] Fix device code instrumentation by AGindinson · Pull Request #4615 · intel/llvm

AGindinson · 2021-09-22T06:54:51Z

Fix the malfunctioning driver option for passing -fsycl-instrument-device-code to CC1
for SPIR-V-based targets.
Add the ITT device libraries when the CLI option is enabled, excluding these from
-fno-sycl-device-lib effects.
Disable ITT annotations for ESIMD code.

Signed-off-by: Artem Gindinson artem.gindinson@intel.com

clang/test/Driver/sycl-instrumentation.c

AGindinson · 2021-09-22T10:53:17Z

Moving to draft - it is possible that defaulting the FE option to true and removing the malfunctioning driver option entirely should be done immediately.

Instead of maintaining a (malfunctioning) driver option, unconditionally pass the CC1 option `-fsycl-instrument-device-code` for SPIR-V-based targets. Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

premanandrao

Looks okay to me.

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

clang/lib/Driver/Driver.cpp

llvm/lib/SYCLLowerIR/LowerESIMD.cpp

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

The addition of internal libraries cannot be affected by `-fno-sycl-device-lib` Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

AGindinson · 2021-09-27T16:35:07Z

Still TBD:

Add an E2E test with device code instrumentation ON
Address Konst's comment

REMOVED: "Rework the parsing of the CLI option so that the non-SPIR-triple error is emitted both for compile and link phases"
(not needed for now since device libraries are only used for SPIR-V based targets)

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

Instead of skipping the annotations during ESIMD-specific module processing, don't add the instrumentation to these GPU-specific kernels in the first place. Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

Add a couple tests with execute-only kernels to ensure that ITT annotations' generation doesn't break the compilation/execution. This aims to test intel/llvm#4615. Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

AGindinson · 2021-09-28T11:29:45Z

@premanandrao @elizabethandrews @AaronBallman Just FYI, there are no changes expected to Clang FE with this revision of the PR, so feel free to unsubscribe if the notifications are bugging you.

premanandrao · 2021-09-28T14:38:31Z

@premanandrao @elizabethandrews @AaronBallman Just FYI, there are no changes expected to Clang FE with this revision of the PR, so feel free to unsubscribe if the notifications are bugging you.

Thanks for the note, but no worries. :-)

clang/lib/Driver/ToolChains/Clang.cpp

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

vzakhari

SPIRITTAnnotationsPass changes LGTM.

mdtoguchi

LGTM

AGindinson · 2021-09-28T18:25:34Z

The Precommit checks have passed after being tuned to the latest revision of intel/llvm-test-suite#484, approvals for major changes have been gathered. @kbobrovs, could you please review the changes in sycl/test/esimd/vadd.cpp RUN lines and the comment resolution?

* sycl: (108 commits) [SYCL][XPTI] Revisit resource management strategy (intel#4494) [SYCL][ESIMD] Fix misprint: ESIMD_L1_FLUASH_RO_DATA -> ESIMD_L1_FLUSH_RO_DATA (intel#4681) [SYCL] Make kernel_bundle interop more conformant (intel#4672) [SYCL] Submission with kernel parameter ignores set kernel bundle (intel#4667) [SYCL] Add support for std::byte to vec class (intel#4637) [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#4671) [SYCL] Fix an error on host when big image is used on opencl:gpu (intel#4668) [SYCL] Exclude exported symbols from kernel bundles (intel#4660) Revert "[SYCL] Allow overriding plugin libraries (intel#4067)" (intel#4659) [SYCL] Handle exceptions on mutually exclusive handler operations (intel#4639) [sycl-post-link] Don't split module if function pointer has a user that's not CallInst (intel#4657) [SYCL][HIP] Fix MemBufferFill for nvidia platform (intel#4629) [SYCL][Doc] Describe DPC++ CUDA install w/ non-standard toolkit loc (intel#4663) [SYCL] Fix device code instrumentation (intel#4615) Remove myself as a code owner (intel#4653) [SYCL] Fix overwriting insert to sub_group_mask (intel#4656) [x86][Matrix] Replace packed_a with row_major in matrix testcases (intel#4641) [SYCL][Doc] Add device global extension spec (intel#4233) [sycl-post-link] Adds property listing exported functions (intel#4626) [Driver][SYCL] Do not consider non-archive files for FPGA binary checks (intel#4644) ...

* sycl: (107 commits) [SYCL][XPTI] Revisit resource management strategy (intel#4494) [SYCL][ESIMD] Fix misprint: ESIMD_L1_FLUASH_RO_DATA -> ESIMD_L1_FLUSH_RO_DATA (intel#4681) [SYCL] Make kernel_bundle interop more conformant (intel#4672) [SYCL] Submission with kernel parameter ignores set kernel bundle (intel#4667) [SYCL] Add support for std::byte to vec class (intel#4637) [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#4671) [SYCL] Fix an error on host when big image is used on opencl:gpu (intel#4668) [SYCL] Exclude exported symbols from kernel bundles (intel#4660) Revert "[SYCL] Allow overriding plugin libraries (intel#4067)" (intel#4659) [SYCL] Handle exceptions on mutually exclusive handler operations (intel#4639) [sycl-post-link] Don't split module if function pointer has a user that's not CallInst (intel#4657) [SYCL][HIP] Fix MemBufferFill for nvidia platform (intel#4629) [SYCL][Doc] Describe DPC++ CUDA install w/ non-standard toolkit loc (intel#4663) [SYCL] Fix device code instrumentation (intel#4615) Remove myself as a code owner (intel#4653) [SYCL] Fix overwriting insert to sub_group_mask (intel#4656) [x86][Matrix] Replace packed_a with row_major in matrix testcases (intel#4641) [SYCL][Doc] Add device global extension spec (intel#4233) [sycl-post-link] Adds property listing exported functions (intel#4626) [Driver][SYCL] Do not consider non-archive files for FPGA binary checks (intel#4644) ...

AGindinson requested a review from MrSidims September 22, 2021 06:54

AGindinson requested review from hchilama and mdtoguchi as code owners September 22, 2021 06:54

MrSidims requested review from alexbatashev and romanovvlad September 22, 2021 07:27

MrSidims reviewed Sep 22, 2021

View reviewed changes

clang/test/Driver/sycl-instrumentation.c Outdated Show resolved Hide resolved

MrSidims previously approved these changes Sep 22, 2021

View reviewed changes

AGindinson dismissed MrSidims’s stale review via c18fd39 September 22, 2021 07:35

MrSidims previously approved these changes Sep 22, 2021

View reviewed changes

AGindinson marked this pull request as draft September 22, 2021 10:53

[SYCL] Unconditionally enable SPIR ITT annotations

d23adc8

Instead of maintaining a (malfunctioning) driver option, unconditionally pass the CC1 option `-fsycl-instrument-device-code` for SPIR-V-based targets. Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

AGindinson dismissed MrSidims’s stale review via d23adc8 September 22, 2021 16:23

AGindinson force-pushed the instr-device-code branch from c18fd39 to d23adc8 Compare September 22, 2021 16:23

AGindinson changed the title ~~[SYCL][Driver] Fix device code instrumentation~~ [SYCL] Unconditionally enable SPIR ITT annotations Sep 22, 2021

AGindinson marked this pull request as ready for review September 22, 2021 16:25

AGindinson requested review from AaronBallman, elizabethandrews and premanandrao as code owners September 22, 2021 16:25

AGindinson requested a review from MrSidims September 22, 2021 16:25

MrSidims previously approved these changes Sep 22, 2021

View reviewed changes

premanandrao previously approved these changes Sep 22, 2021

View reviewed changes

mdtoguchi previously approved these changes Sep 22, 2021

View reviewed changes

Artem Gindinson added 2 commits September 23, 2021 08:09

Merge remote-tracking branch 'intel/sycl' into instr-device-code

48327b4

Add ITT device libs to the default linkage group

fe3becc

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

AGindinson dismissed stale reviews from mdtoguchi, premanandrao, and MrSidims via fe3becc September 23, 2021 09:40

AGindinson requested review from MrSidims and premanandrao September 23, 2021 09:40

mdtoguchi reviewed Sep 23, 2021

View reviewed changes

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

kbobrovs reviewed Sep 23, 2021

View reviewed changes

llvm/lib/SYCLLowerIR/LowerESIMD.cpp Outdated Show resolved Hide resolved

Artem Gindinson added 4 commits September 27, 2021 18:16

Return to explicit enabling of ITT annotations via CLI

ad89167

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

Add ITT device libraries as "internal" when the ITT option is provided

cb0a1ea

The addition of internal libraries cannot be affected by `-fno-sycl-device-lib` Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

Add tests for the preceding 2 commits

bbb028c

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

Merge remote-tracking branch 'intel/sycl' into instr-device-code

f9d4b5b

AGindinson changed the title ~~[SYCL] Unconditionally enable SPIR ITT annotations~~ [SYCL] Fix device code instrumentation Sep 27, 2021

Artem Gindinson added 3 commits September 27, 2021 20:35

Fix FileCheck regexp for OS-dependent cases

ba04b59

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

Add a TODO for devicelib-related refactoring

47e77f2

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

Do not instrument ESIMD functions

657c7b6

Instead of skipping the annotations during ESIMD-specific module processing, don't add the instrumentation to these GPU-specific kernels in the first place. Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

AGindinson requested a review from vzakhari as a code owner September 28, 2021 09:38

AGindinson mentioned this pull request Sep 28, 2021

[SYCL] Add E2E tests for device code instrumentation intel/llvm-test-suite#484

Merged

AGindinson requested review from kbobrovs and mdtoguchi September 28, 2021 11:28

mdtoguchi reviewed Sep 28, 2021

View reviewed changes

clang/lib/Driver/ToolChains/Clang.cpp Outdated Show resolved Hide resolved

Change argument parsing logic & update test accordingly

fcbf00a

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

AGindinson requested a review from mdtoguchi September 28, 2021 15:25

vzakhari approved these changes Sep 28, 2021

View reviewed changes

MrSidims approved these changes Sep 28, 2021

View reviewed changes

mdtoguchi approved these changes Sep 28, 2021

View reviewed changes

kbobrovs approved these changes Sep 29, 2021

View reviewed changes

AlexeySachkov merged commit 5667aba into intel:sycl Sep 29, 2021

AGindinson deleted the instr-device-code branch January 1, 2023 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Fix device code instrumentation#4615

[SYCL] Fix device code instrumentation#4615
AlexeySachkov merged 14 commits intointel:syclfrom
AGindinson:instr-device-code

AGindinson commented Sep 22, 2021 •

edited

Loading

Uh oh!

Uh oh!

AGindinson commented Sep 22, 2021

Uh oh!

premanandrao left a comment

Uh oh!

Uh oh!

Uh oh!

AGindinson commented Sep 27, 2021 •

edited

Loading

Uh oh!

AGindinson commented Sep 28, 2021

Uh oh!

premanandrao commented Sep 28, 2021

Uh oh!

Uh oh!

vzakhari left a comment

Uh oh!

mdtoguchi left a comment

Uh oh!

AGindinson commented Sep 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

AGindinson commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AGindinson commented Sep 22, 2021

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AGindinson commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AGindinson commented Sep 28, 2021

Uh oh!

premanandrao commented Sep 28, 2021

Uh oh!

Uh oh!

vzakhari left a comment

Choose a reason for hiding this comment

Uh oh!

mdtoguchi left a comment

Choose a reason for hiding this comment

Uh oh!

AGindinson commented Sep 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AGindinson commented Sep 22, 2021 •

edited

Loading

AGindinson commented Sep 27, 2021 •

edited

Loading