[SYCL] Mark kernel on host for enabling optimizations. #2186

premanandrao · 2020-07-27T22:04:21Z

To enable better optimizations on the host, kernel implementation
routines may be marked with the attribute [[clang::sycl_kernel]].
Previously the 'sycl_kernel' attribute was ignored on host.
The call operator of the function object passed to the SYCL kernel
is also marked with 'alwaysinline' on the host to enable these optimizations
to be more effective.

Signed-off-by: Premanand M Rao premanand.m.rao@intel.com

premanandrao · 2020-07-27T22:05:42Z

TODO: When PR #1785 is merged, the tests need to be modified to use the new const reference syntax.

Fznamznon · 2020-07-28T09:10:53Z

clang/lib/Sema/SemaDeclAttr.cpp

+  // Function must return void.
+  if (!getFunctionOrMethodResultType(D)->isVoidType()) {
+    S.Diag(FT->getLocation(), diag::warn_sycl_attr_return_type)
+        << "sycl_kernel_impl";


Why do you need additional attribute?
sycl_kernel_impl looks fully identical to sycl_kernel attribute. Can we just use sycl_kernel attribute instead?

The main issue is the restriction on the number of template arguments and the number of function arguments on a routine marked "sycl_kernel". The implementation routines that I have seen seem to need a bit more leeway than that. Can that restriction on the "sycl_kernel" routines be lifted? If so, we can reuse the same attribute.

I meant "parameters" in both places that I said "arguments" above.

Those restrictions are not coming from the spec, they aim to guarantee that the right function is marked with this attribute, so it can be used to construct OpenCL kernel.
We can lift some of them if we are sure that OpenCL kernel generation won't be broken.

Thanks for this information, @Fznamznon! Since OpenCL kernel generation happens on the device, I will try to reuse the same attribute on the host for this PR purpose.

clang/test/CodeGenSYCL/sycl_kernel-host.cpp

Fznamznon · 2020-07-29T09:28:49Z

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

+    S.ConstructOpenCLKernel(FD, MC);
+  } else if (S.LangOpts.SYCLIsHost) {
+    auto CRD = (*FD->param_begin())->getType()->getAsCXXRecordDecl();
+    for (auto Method : CRD->methods())


Suggested change

for (auto Method : CRD->methods())

for (auto *Method : CRD->methods())

Fznamznon · 2020-07-29T09:29:33Z

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

+  // Because all SYCL kernel functions are template functions - they
+  // have deferred instantination. We need bodies of these functions
+  // so we are checking for SYCL kernel attribute after instantiation.


I think this comment became out of context here. Lets remove it.

Fznamznon · 2020-07-29T09:34:05Z

clang/test/SemaSYCL/sycl_kernel-host.cpp

@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -fsycl -fsycl-is-host -fsyntax-only -verify %s


Can we merge this test with existing test/SemaSYCL/kernel-attribute.cpp test?

Done. Please review.

Fznamznon · 2020-07-29T09:35:04Z

clang/lib/CodeGen/CodeGenFunction.cpp

@@ -891,6 +891,12 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
  if (D && D->hasAttr<CFICanonicalJumpTableAttr>())
    Fn->addFnAttr("cfi-canonical-jump-table");

+  if (getLangOpts().SYCLIsHost && D && D->hasAttr<SYCLKernelAttr>()) {
+    Fn->addFnAttr("sycl_kernel");
+    // Sycl kernel functions are not subject to inlining


Suggested change

// Sycl kernel functions are not subject to inlining

// SYCL kernel functions are not subject to inlining.

Fznamznon · 2020-07-29T09:37:06Z

clang/test/CodeGenSYCL/sycl_kernel-host.cpp

+// Test that the kernel implementation routine marked with 'sycl_kernel' has the attribute `sycl_kernel` in the generated LLVM IR
+// and that the function object passed to the sycl kernel is marked 'alwaysinline' on the host


Can you please keep these lines short? i.e. 80 characters.

Done, sorry I missed this.

Fznamznon · 2020-07-29T09:39:17Z

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

+  if (S.LangOpts.SYCLIsDevice) {
+    S.ConstructOpenCLKernel(FD, MC);
+  } else if (S.LangOpts.SYCLIsHost) {
+    auto CRD = (*FD->param_begin())->getType()->getAsCXXRecordDecl();


Suggested change

auto CRD = (*FD->param_begin())->getType()->getAsCXXRecordDecl();

auto *CRD = (*FD->param_begin())->getType()->getAsCXXRecordDecl();

However, I would prefer explicit type here.

Used explicit type here.

keryell · 2020-07-29T12:29:16Z

If the goal is to have [[always_inline]] for example, just use it inside the C++ headers and do not touch the compiler at all?

premanandrao · 2020-07-29T16:48:21Z

If the goal is to have [[always_inline]] for example, just use it inside the C++ headers and do not touch the compiler at all?

The 'alwaysinline' attribute is applied on the call operator of the user-provided lambda or function object. I am not sure how that can be marked in the header file. If I have misunderstood you, please explain.
However the 'noinline' attribute was on the sycl kernel itself and I have removed it (in my code changes) since that can be done in the header file.

Fznamznon

Small nit, otherwise ok.

Fznamznon · 2020-08-03T11:40:07Z

clang/lib/Sema/SemaDeclAttr.cpp

+  // Function template must have at least two template parameters to be
+  // able to generate OpenCL SYCL kernels.


Suggested change

// Function template must have at least two template parameters to be

// able to generate OpenCL SYCL kernels.

// Function template must have at least two template parameters so it can be used

// in OpenCL kernel generation.

To enable better optimizations on the host, kernel implementation routines may be marked with the attribute [[clang::sycl_kernel_impl]]. The function object passed to the SYCL kernel is also marked with alwaysinline on the host to enable these optimizations to be more effective. Signed-off-by: Premanand M Rao <premanand.m.rao@intel.com>

Signed-off-by: Premanand M Rao <premanand.m.rao@intel.com>

premanandrao · 2020-08-03T19:38:33Z

Sorry for the force-push, there were some unrelated test failures which needed changes from other commits.

Fznamznon · 2020-08-04T07:58:18Z

Tests still failing.

premanandrao · 2020-08-04T15:01:19Z

Tests still failing.

Yes, not sure why though. They were unrelated to my changes and a restart shows a clean run.

premanandrao · 2020-08-05T15:46:35Z

@bader , would it be possible to merge this soon? I'd like this to get into github today if feasible. Thanks!

This is essentially a revert of PR intel#2186, as the supposed optimization did not materialize. I have removed the functionality added in that PR, while keeping the code cleanups.

This is essentially a revert of PR #2186, as the supposed optimization did not materialize. I have removed the functionality added in that PR, while keeping the code cleanups.

premanandrao requested review from elizabethandrews and Fznamznon as code owners July 27, 2020 22:04

premanandrao requested a review from turinevgeny July 27, 2020 22:23

Fznamznon reviewed Jul 28, 2020

View reviewed changes

premanandrao requested a review from Fznamznon July 28, 2020 19:39

Fznamznon reviewed Jul 29, 2020

View reviewed changes

Fznamznon reviewed Aug 3, 2020

View reviewed changes

premanandrao added 6 commits August 3, 2020 12:01

[SYCL] Make clang-format changes.

ea6dc8a

Signed-off-by: Premanand M Rao <premanand.m.rao@intel.com>

[SYCL] Make clang-format change.

60b7354

Signed-off-by: Premanand M Rao <premanand.m.rao@intel.com>

[SYCL] Reuse sycl_kernel attribute

bafd07d

Signed-off-by: Premanand M Rao <premanand.m.rao@intel.com>

[SYCL] Address code-review comments

9dac861

Signed-off-by: Premanand M Rao <premanand.m.rao@intel.com>

Address code-review comment

690af65

premanandrao force-pushed the remote_sycl_kernel_impl branch from 2eb8142 to 690af65 Compare August 3, 2020 19:35

premanandrao requested a review from Fznamznon August 3, 2020 19:38

Fznamznon approved these changes Aug 4, 2020

View reviewed changes

bader merged commit 414c1e5 into intel:sycl Aug 5, 2020

srividya-sundaram mentioned this pull request Sep 16, 2020

[SYCL] Remove diagnostics emission from Integration header functionality #2474

Merged

premanandrao mentioned this pull request Sep 22, 2021

[SYCL] Disallow sycl_kernel attribute on host #4618

Merged

	for (auto Method : CRD->methods())
	for (auto *Method : CRD->methods())

		@@ -0,0 +1,37 @@
		// RUN: %clang_cc1 -fsycl -fsycl-is-host -fsyntax-only -verify %s

	// Sycl kernel functions are not subject to inlining
	// SYCL kernel functions are not subject to inlining.

		// Test that the kernel implementation routine marked with 'sycl_kernel' has the attribute `sycl_kernel` in the generated LLVM IR
		// and that the function object passed to the sycl kernel is marked 'alwaysinline' on the host

	auto CRD = (*FD->param_begin())->getType()->getAsCXXRecordDecl();
	auto CRD = (FD->param_begin())->getType()->getAsCXXRecordDecl();

		// Function template must have at least two template parameters to be
		// able to generate OpenCL SYCL kernels.

[SYCL] Mark kernel on host for enabling optimizations. #2186

[SYCL] Mark kernel on host for enabling optimizations. #2186

Uh oh!

Conversation

premanandrao commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

premanandrao commented Jul 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keryell commented Jul 29, 2020

Uh oh!

premanandrao commented Jul 29, 2020

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

premanandrao commented Aug 3, 2020

Uh oh!

Fznamznon commented Aug 4, 2020

Uh oh!

premanandrao commented Aug 4, 2020

Uh oh!

premanandrao commented Aug 5, 2020

Uh oh!

Uh oh!

premanandrao commented Jul 27, 2020 •

edited

Loading