[SYCL] Enable host optimization of work-item free functions #2967

cperkinsintel · 2020-12-29T19:45:45Z

The SYCL free functions ( this_item, this_id, etc) are expensive to support on host devices. They cause performance delays because every iteration through one of the parallel_for routines the various indexing values have to be updated in case the users code might call this_item or this_id (or the others). But with the new callsThisItem method added to the Kernel Information (thanks @rdeodhar !), the host device can avoid paying the performance penalty if the users code doesn't actually call this_item.. We can detect at compile time whether or not any of the this_xxx free functions are used by the users code, and if not, don't bother storing the indexing data in each loop iteration.

In this PR we add further expand the Kernel Information to support a callsAnyThisFreeFunction method, and we use it to avoid the sundry store_item etc. calls on the host.

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

…ons and checking for any usage to optimize host kernel tasks. Signed-off-by: Chris Perkins <chris.perkins@intel.com>

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

sycl/include/CL/sycl/detail/cg_types.hpp

Fznamznon · 2020-12-30T14:22:07Z

clang/lib/Sema/SemaSYCL.cpp

+    O << (K.FreeFunctionCalls.CallsThisId ||
+          K.FreeFunctionCalls.CallsThisItem ||
+          K.FreeFunctionCalls.CallsThisNDItem ||
+          K.FreeFunctionCalls.CallsThisGroup)
+      << "; }\n";


It feels that if the final information in the integration header doesn't say which exactly free function is called, we don't need four different flags and four identical handlers (I mean SYCLIntegrationHeader::setCallsThisItem and others here) in front-end either.

On the other hand I guess the host runtime could be optimized if we knows exactly which one is used...

@intel/llvm-reviewers-runtime , WDYT?

Pre-existing code unrelated to this PR needs to know if this_item is called and that isn't concerned about the other free functions. The optimization in this PR just needs to know if any of them are called.
My thoughts are that it's best to make a proper record of each free function now. Recording only "this_item" and "anything" seems sloppy.
On the API side, though, the functions match our current needs. The pre-existing callsThisItem() and callsAnyThisFreeFunction() are the two affordances, but can be easily expanded in the future if required.

Do you expect it to be required?

I don't know of anything pending. But I'd wager at even odds on needing to know about this_nd_item or this_group usage in the future.

Alright thanks! I'm ok with the FE changes.

keryell

You can also update the PR description which is completely cryptic.
Perhaps with "Enable host optimization of work-item free functions" for the title and a few paragraphs of what is happening here and why.

clang/include/clang/Sema/Sema.h

clang/lib/Sema/SemaSYCL.cpp

keryell · 2020-12-30T14:59:22Z

clang/lib/Sema/SemaSYCL.cpp

+    O << (K.FreeFunctionCalls.CallsThisId ||
+          K.FreeFunctionCalls.CallsThisItem ||
+          K.FreeFunctionCalls.CallsThisNDItem ||
+          K.FreeFunctionCalls.CallsThisGroup)
+      << "; }\n";


On the other hand I guess the host runtime could be optimized if we knows exactly which one is used...

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

clang/include/clang/Sema/Sema.h

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

elizabethandrews

FE changes LGTM, other than the unresolved comment. I am alright with keeping the 4 different flags for now, if we expect to differentiate between free functions in the future.

elizabethandrews · 2020-12-31T15:29:20Z

clang/lib/Sema/SemaSYCL.cpp

+    O << (K.FreeFunctionCalls.CallsThisId ||
+          K.FreeFunctionCalls.CallsThisItem ||
+          K.FreeFunctionCalls.CallsThisNDItem ||
+          K.FreeFunctionCalls.CallsThisGroup)
+      << "; }\n";


Do you expect it to be required?

elizabethandrews

FE changes LGTM.

elizabethandrews · 2021-01-04T18:22:18Z

clang/lib/Sema/SemaSYCL.cpp

+    O << (K.FreeFunctionCalls.CallsThisId ||
+          K.FreeFunctionCalls.CallsThisItem ||
+          K.FreeFunctionCalls.CallsThisNDItem ||
+          K.FreeFunctionCalls.CallsThisGroup)
+      << "; }\n";


Alright thanks! I'm ok with the FE changes.

sycl/include/CL/sycl/handler.hpp

sycl/include/CL/sycl/detail/cg_types.hpp

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

elizabethandrews

FE changes LGTM

v-klochkov

Looks Good To Me.
I also did not find any changes that would break backward compatibility.

rbegam

sycl changes LGTM.
there is a pre-commit fail though.

pvchupin · 2021-01-08T00:21:11Z

fail is flaky, @v-klochkov faced the same elsewhere and going to open issue.

With opaque pointers, this is equivalent to calling `IRBuilder::CreateAddrSpaceCast`. Original commit: KhronosGroup/SPIRV-LLVM-Translator@0d3a1a207dc6dc5

cperkinsintel added 8 commits December 21, 2020 17:44

initial commit for time trials.

6f79af1

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

expanding callsThisItem functionality to include all four free functi…

a30131c

…ons and checking for any usage to optimize host kernel tasks. Signed-off-by: Chris Perkins <chris.perkins@intel.com>

updated tests

15d5bf4

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

overlooked comment

3419fb6

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

clang-format continues to declare its devotion. This is my tender reply.

9d2d843

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

clang-format appreciates little my penitent reply

a05c667

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

restore original test invocation line

8526e5e

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

updated expectation line no. due to change in proxy sycl.hpp

da71fe9

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

cperkinsintel marked this pull request as ready for review December 30, 2020 00:30

cperkinsintel requested review from elizabethandrews, Fznamznon, premanandrao and a team as code owners December 30, 2020 00:30

cperkinsintel requested review from rbegam and rdeodhar December 30, 2020 00:30

Fznamznon reviewed Dec 30, 2020

View reviewed changes

keryell suggested changes Dec 30, 2020

View reviewed changes

cperkinsintel changed the title ~~[SYCL] free function host optimization~~ [SYCL] Enable host optimization of work-item free functions Dec 30, 2020

changes requested by reviewers

0ecd023

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

elizabethandrews reviewed Dec 30, 2020

View reviewed changes

clang/include/clang/Sema/Sema.h Outdated Show resolved Hide resolved

cperkinsintel added 2 commits December 30, 2020 11:39

reviewer requests

2841252

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

clang-format <3 4-evah

c43160f

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

elizabethandrews reviewed Dec 31, 2020

View reviewed changes

elizabethandrews previously approved these changes Jan 4, 2021

View reviewed changes

v-klochkov reviewed Jan 5, 2021

View reviewed changes

sycl/include/CL/sycl/handler.hpp Outdated Show resolved Hide resolved

v-klochkov reviewed Jan 5, 2021

View reviewed changes

sycl/include/CL/sycl/detail/cg_types.hpp Outdated Show resolved Hide resolved

changes requested in review

80453fd

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

cperkinsintel dismissed elizabethandrews’s stale review via 80453fd January 5, 2021 19:24

more requested changes. put get_kernel_name_t decl in one place

f05da87

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

elizabethandrews approved these changes Jan 5, 2021

View reviewed changes

v-klochkov approved these changes Jan 6, 2021

View reviewed changes

rbegam reviewed Jan 7, 2021

View reviewed changes

pvchupin merged commit b83a1a8 into intel:sycl Jan 8, 2021

premanandrao mentioned this pull request Jan 8, 2021

[SYCL] Add identification macro for DPCPP compiler #2998

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Enable host optimization of work-item free functions #2967

[SYCL] Enable host optimization of work-item free functions #2967

cperkinsintel commented Dec 29, 2020 •

edited

Loading

Fznamznon Dec 30, 2020 •

edited

Loading

keryell Dec 30, 2020

Fznamznon Dec 30, 2020

cperkinsintel Dec 30, 2020

elizabethandrews Dec 31, 2020

cperkinsintel Jan 4, 2021

elizabethandrews Jan 4, 2021

keryell left a comment

keryell Dec 30, 2020

elizabethandrews left a comment

elizabethandrews Dec 31, 2020

elizabethandrews left a comment

elizabethandrews Jan 4, 2021

elizabethandrews left a comment

v-klochkov left a comment

rbegam left a comment •

edited

Loading

pvchupin commented Jan 8, 2021

[SYCL] Enable host optimization of work-item free functions #2967

[SYCL] Enable host optimization of work-item free functions #2967

Conversation

cperkinsintel commented Dec 29, 2020 • edited Loading

Fznamznon Dec 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keryell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elizabethandrews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elizabethandrews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elizabethandrews left a comment

Choose a reason for hiding this comment

v-klochkov left a comment

Choose a reason for hiding this comment

rbegam left a comment • edited Loading

Choose a reason for hiding this comment

pvchupin commented Jan 8, 2021

cperkinsintel commented Dec 29, 2020 •

edited

Loading

Fznamznon Dec 30, 2020 •

edited

Loading

rbegam left a comment •

edited

Loading