[SYCL] Propagate attributes of original kernel to wrapper kernel generated for range-rounding #3306

rdeodhar · 2021-03-04T23:31:15Z

This change propagates attributes of a user-written SYCL kernel to the kernel generated as a wrapper around the original kernel. The wrapped kernel is executed with the original range rounded-up, which improves work group formation on CPUs and GPUs.

Signed-off-by: rdeodhar rajiv.deodhar@intel.com

Signed-off-by: rdeodhar <rajiv.deodhar@intel.com>

bader

Please, cherry-pick 496f9a0 to this pull request to fix the checks.

…s when rounding up the range (intel#3154) Parallel_for range rounding creates a wrapped version of the original kernel. Kernel attributes need to be copied from original kernel to new wrapped kernel. Signed-off-by: rdeodhar <rajiv.deodhar@intel.com>

elizabethandrews · 2021-03-08T18:27:14Z

@rdeodhar please fix the conflicts

Signed-off-by: rdeodhar <rajiv.deodhar@intel.com>

elizabethandrews · 2021-03-08T20:34:34Z

@rdeodhar please update commit message and title to reflect the re-landing of this patch. Thanks!

@AaronBallman

No objections from my side, I'd like @AaronBallman to take a look.

AaronBallman · 2021-03-09T12:27:22Z

clang/lib/Sema/SemaSYCL.cpp

@@ -306,6 +306,36 @@ static int64_t getIntExprValue(const Expr *E, ASTContext &Ctx) {
  return E->getIntegerConstantExpr(Ctx)->getSExtValue();
 }

+// Collect function attributes related to SYCL


Add a period at the end of the comment.

clang/lib/Sema/SemaSYCL.cpp

AaronBallman · 2021-03-09T12:39:49Z

clang/lib/Sema/SemaSYCL.cpp

+// For a wrapped parallel_for, copy attributes from original
+// kernel to wrapped kernel.
+void Sema::copySYCLKernelAttrs(const CXXRecordDecl *KernelObj) {
+  // Get the operator() function of the wrapper


Add a period to the end of the comment.

AaronBallman

LGTM

clang/lib/Sema/SemaSYCL.cpp

AaronBallman · 2021-03-09T18:39:12Z

clang/test/SemaSYCL/parallel_for_wrapper_attr.cpp

@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 %s -fsyntax-only -ast-dump -fsycl -fsycl-is-device -triple spir64 | FileCheck %s


I don't know how we typically organize test, but in community, tests for -ast-dump typically live in the AST directory.

AaronBallman · 2021-03-09T18:43:13Z

clang/test/SemaSYCL/stall_enable.cpp

@@ -6,7 +6,7 @@
 using namespace cl::sycl;
 queue q;

-[[intel::use_stall_enable_clusters]] void test() {} //expected-warning{{'use_stall_enable_clusters' attribute ignored}}
+[[intel::use_stall_enable_clusters]] void test() {} // expected-warning{{'use_stall_enable_clusters' attribute allowed only on a function directly called from a SYCL kernel}}


Can you use the full attribute diagnostic here (it drops the attribute ignored part, which made me think the diagnostic text was a problem)?

keryell · 2021-03-09T23:45:45Z

At some point we need to consider replacing most of the fancy comfort attributes by just using the C++ type system, for example like http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2183r0.html

So instead of hacking the compiler for each magical class, it is mostly metaprogramming in the C++ run-time library.

AaronBallman · 2021-03-10T15:41:03Z

At some point we need to consider replacing most of the fancy comfort attributes by just using the C++ type system, for example like http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2183r0.html

So instead of hacking the compiler for each magical class, it is mostly metaprogramming in the C++ run-time library.

Strong +1 (many of these attributes feel a bit suspect to me).

romanovvlad

sycl/test/kernel_param/attr.cpp LGTM

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

[SYCL] This test change accompanies PR 3154

db2541b

Signed-off-by: rdeodhar <rajiv.deodhar@intel.com>

rdeodhar requested review from elizabethandrews and premanandrao as code owners March 4, 2021 23:31

bader previously requested changes Mar 5, 2021

View reviewed changes

rdeodhar requested a review from a team as a code owner March 5, 2021 16:55

rdeodhar requested a review from s-kanaev March 5, 2021 16:55

[SYCL] Test change.

3b1ba7c

Signed-off-by: rdeodhar <rajiv.deodhar@intel.com>

elizabethandrews previously approved these changes Mar 8, 2021

View reviewed changes

rdeodhar changed the title ~~[SYCL] This test change accompanies PR 3154~~ [SYCL] Propagate attributes of original kernel to wrapper kernel generated for range-rounding Mar 8, 2021

rdeodhar requested a review from bader March 8, 2021 22:46

bader requested a review from AaronBallman March 9, 2021 07:03

AaronBallman reviewed Mar 9, 2021

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved

AaronBallman reviewed Mar 9, 2021

View reviewed changes

bader removed their request for review March 9, 2021 12:45

Added and edited comments.

a84caa3

rdeodhar dismissed elizabethandrews’s stale review via a84caa3 March 9, 2021 17:19

AaronBallman approved these changes Mar 9, 2021

View reviewed changes

bader requested a review from elizabethandrews March 10, 2021 16:29

elizabethandrews approved these changes Mar 10, 2021

View reviewed changes

romanovvlad approved these changes Mar 16, 2021

View reviewed changes

romanovvlad merged commit 25b482b into intel:sycl Mar 16, 2021

keryell mentioned this pull request Mar 17, 2021

How to make SYCL easily extensible without changing Clang #55

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Propagate attributes of original kernel to wrapper kernel generated for range-rounding #3306

[SYCL] Propagate attributes of original kernel to wrapper kernel generated for range-rounding #3306

rdeodhar commented Mar 4, 2021 •

edited

Loading

bader left a comment •

edited

Loading

elizabethandrews commented Mar 8, 2021

elizabethandrews commented Mar 8, 2021

AaronBallman Mar 9, 2021

rdeodhar Mar 9, 2021

AaronBallman Mar 9, 2021

rdeodhar Mar 9, 2021

AaronBallman left a comment

AaronBallman Mar 9, 2021

AaronBallman Mar 9, 2021

keryell commented Mar 9, 2021

AaronBallman commented Mar 10, 2021

romanovvlad left a comment

		@@ -0,0 +1,54 @@
		// RUN: %clang_cc1 %s -fsyntax-only -ast-dump -fsycl -fsycl-is-device -triple spir64 \| FileCheck %s

[SYCL] Propagate attributes of original kernel to wrapper kernel generated for range-rounding #3306

[SYCL] Propagate attributes of original kernel to wrapper kernel generated for range-rounding #3306

Conversation

rdeodhar commented Mar 4, 2021 • edited Loading

bader left a comment • edited Loading

Choose a reason for hiding this comment

elizabethandrews commented Mar 8, 2021

elizabethandrews commented Mar 8, 2021

AaronBallman Mar 9, 2021

Choose a reason for hiding this comment

rdeodhar Mar 9, 2021

Choose a reason for hiding this comment

AaronBallman Mar 9, 2021

Choose a reason for hiding this comment

rdeodhar Mar 9, 2021

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

AaronBallman Mar 9, 2021

Choose a reason for hiding this comment

AaronBallman Mar 9, 2021

Choose a reason for hiding this comment

keryell commented Mar 9, 2021

AaronBallman commented Mar 10, 2021

romanovvlad left a comment

Choose a reason for hiding this comment

rdeodhar commented Mar 4, 2021 •

edited

Loading

bader left a comment •

edited

Loading