Copy attributes of parallel_for kernels to the wrapped versions when rounding up the range #3154

rdeodhar · 2021-02-03T22:21:51Z

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>

AaronBallman · 2021-02-04T12:55:55Z

clang/lib/Sema/SemaSYCL.cpp

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

+// Collect function attributes related to SYCL
+void CollectSYCLAttributes(Sema &S, FunctionDecl *FD,


This function should be static, named collectSYCLAttributes(), and directly_called should be DirectlyCalled per the usual coding conventions.

Also, the name is a bit misleading because this function does more than just collect attributes - it also issues diagnostics and modifies the given function in some cases.

OK on the name changes.

AaronBallman · 2021-02-04T13:05:20Z

clang/lib/Sema/SemaSYCL.cpp

+void CollectSYCLAttributes(Sema &S, FunctionDecl *FD,
+                           llvm::SmallPtrSet<Attr *, 4> &Attrs,
+                           bool directly_called = true) {
+  if (auto *A = FD->getAttr<IntelReqdSubGroupSizeAttr>())


I think this can be simplified into:

llvm::copy_if(FD->getAtts(), std::inserter(Attrs), [](Attr *A) { return isa<IntelReqdSubGroupSizeAttr, ReqdWorkGroupSizeAttr, SYCLIntelKernelArgsRestrictAttr, etc>(A); });

This is functionally different in that the approach in the patch will only add the attribute to the container once whereas the looping approach will get all of the attributes if any of them have been duplicated.

It may be worth it to consider splitting this into two functions. One that collects all the copies of everything but SYCLIntelUseStallEnableClustersAttr, and one that uses FD->specific_attrs<SYCLIntelUseStallEnableClustersAttr>() to loop over just those attributes and handle them.

Thinking on this a bit more... it would be best if we were able to use the table generator to implement the logical guts of this function. This way, we could add something to each attribute definition in Attr.td, like let CopyToKernel = 1;, to signify which attributes opt into this behavior. Then ClangAttrEmitter.cpp could be modified to spit out a new .inc file that we #include within the llvm::copy_if() lambda. On the assumption that most attributes will not need custom logic like SYCLIntelUseStallEnableClustersAttr does, this could make it easier to opt attributes into this functionality in the future. WDYT? (To be clear, I'm not suggesting this tablegen work as part of this patch but instead as a follow-up refactoring. So I'm mostly wondering if that idea is worth opening an issue for or not.)

I am unable to get the llvm::copy_if call to compile. Is there an example of similar usage elsewhere?

Ugh, I didn't realize that llvm::SmallPtrSet<> doesn't have the correct interface for its insert() method -- it doesn't have one accepting an iterator hint. I realized that after noticing that my call to std::inserter() left off the iterator argument (sorry about that). See https://github.com/intel/llvm/blob/sycl/llvm/include/llvm/ADT/SmallPtrSet.h#L364

I actually think this is a deficiency in the upstream implementation. I think SmallPtrSetImpl should be given a function like:

std::pair<iterator, bool> insert(iterator, PtrType Ptr) { return insert(Ptr); }

where the hint is accepted and ignored.

However, I can understand not wanting to do that as part of this patch, so an alternative suggestion is to do the loop copy manually:

for (Attr *A : FD->getAttrs()) { if (isa<IntelReqdSubGroupSizeAttr, ReqdWorkGroupSizeAttr, SYCLIntelKernelArgsRestrictAttr, etc>(A)) Attrs.insert(A); }

OK, thanks for the tip.

FWIW: https://reviews.llvm.org/D96134

Thanks for following up on the need for an inserter.

clang/lib/Sema/SemaSYCL.cpp

AaronBallman

Should there be some test coverage for the changes?

clang/lib/Sema/SemaSYCL.cpp

AaronBallman · 2021-02-05T18:57:09Z

clang/lib/Sema/SemaSYCL.cpp

@@ -3155,6 +3163,62 @@ void Sema::CheckSYCLKernelCall(FunctionDecl *KernelFunc, SourceRange CallLoc,
    KernelFunc->setInvalidDecl();
 }

+// For a wrapped parallel_for, copy attributes from original
+// kernel to wrapped kernel.
+void Sema::copyAttributes(const CXXRecordDecl *KernelObj) {


This function name is a bit generic for the functionality it provides. Should it perhaps be copySYCLKernelAttrs() instead? That's still a bit confusing because there's no mention of copying them to what...

OK, that's a better name.

clang/lib/Sema/SemaSYCL.cpp

AaronBallman · 2021-02-05T19:04:53Z

clang/lib/Sema/SemaSYCL.cpp

  // Calculate both names, since Integration headers need both.
  std::string CalculatedName, StableName;
  std::tie(CalculatedName, StableName) =
      constructKernelName(*this, KernelCallerFunc, MC);
  StringRef KernelName(getLangOpts().SYCLUnnamedLambda ? StableName
                                                       : CalculatedName);
+
+  std::string PF("__pf_kernel_wrapper");


No need for the local variable here.

clang/lib/Sema/SemaSYCL.cpp

AaronBallman · 2021-02-08T13:31:30Z

clang/lib/Sema/SemaSYCL.cpp

+                                  bool DirectlyCalled = true) {
+  if (!FD->hasAttrs())
+    return;
+  for (Attr *A : FD->getAttrs()) {


Given the switch from SmallPtrSet to SmallVector, this can be switched to an llvm::copy_if() (using a std::back_inserter()).

clang/lib/Sema/SemaSYCL.cpp

AaronBallman · 2021-02-08T13:49:38Z

sycl/test/kernel_param/attr.cpp

+  return 0;
+}
+
+// CHECK: define {{.*}}spir_kernel void @{{.*}}__pf_kernel_wrapper{{.*}}reqd_work_group_size


Please add a newline to the end of the file.

sycl/test/kernel_param/attr.cpp

AaronBallman

LGTM, but don't forget to open an issue about the diagnostic behavior from the existing code. Thanks!

AaronBallman · 2021-02-09T13:17:31Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -3192,6 +3192,9 @@ def warn_dllimport_dropped_from_inline_function : Warning<
  InGroup<IgnoredAttributes>;
 def warn_attribute_ignored : Warning<"%0 attribute ignored">,
  InGroup<IgnoredAttributes>;
+def warn_attribute_on_direct_kernel_callee_only : Warning<"%0 attribute allowed"
+  " only on function directly called from kernel; attribute ignored">,


only on function -> only on a function

"called from kernel" is also a bit awkward. called from a kernel? called from a kernel function?

elizabethandrews

Can you add a FE test? I suspect you'll have to add something to test headers to generate the wrapper kernel.

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/lib/Sema/SemaSYCL.cpp

AaronBallman

LGTM!

elizabethandrews

I apologize for the delay in review. I was under the impression I reviewed your latest change. I didn't notice the new commit.

elizabethandrews · 2021-02-17T12:45:30Z

clang/test/SemaSYCL/parallel_for_wrapper_attr.cpp

@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 %s -fsyntax-only -ast-dump -fsycl -fsycl-is-device -triple spir64 | FileCheck %s
+
+template <typename Name, typename Type>


As per new contributing guidelines (https://github.com/intel/llvm/blob/sycl/CONTRIBUTING.md), tests have to include mock headers if required. Is it possible to remove these definitions and modify test to follow guidelines? Please see section "Guidelines for adding DPC++ in-tree LIT tests"

OK, I have moved some definitions into the fake sycl.hpp.

clang/test/SemaSYCL/parallel_for_wrapper_attr.cpp

elizabethandrews

LGTM. Thanks!

… versions when rounding up the range (#3154)" This reverts commit 496f9a0.

… versions when rounding up the range (#3154)" (#3293) This reverts commit 496f9a0.

…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>

rdeodhar added 2 commits February 3, 2021 14:13

[SYCL] Propagate parallel_for kernel attributes to wrapped kernel

ec5b76a

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

Merge branch 'sycl' of https://github.com/intel/llvm into iwgo8

21ff7be

rdeodhar requested review from elizabethandrews and premanandrao as code owners February 3, 2021 22:21

Formatting change.

8679cee

rdeodhar marked this pull request as draft February 3, 2021 22:34

Formatting change.

ca592f3

AaronBallman reviewed Feb 4, 2021

View reviewed changes

rdeodhar added 2 commits February 5, 2021 09:59

Improved scan over function attributes.

d59cfd5

Formatting change.

648ca30

AaronBallman reviewed Feb 5, 2021

View reviewed changes

rdeodhar added 3 commits February 5, 2021 15:30

Added test and changed attribute copying.

392f239

Merge branch 'sycl' of https://github.com/intel/llvm into iwgo8

deba868

Corrected merge error.

75c5f9d

AaronBallman reviewed Feb 8, 2021

View reviewed changes

New attributes appended instead of set to wrapper; other minor changes.

fd93cd1

AaronBallman previously approved these changes Feb 8, 2021

View reviewed changes

Made stall_enable disgnostic more descriptive.

4191d12

rdeodhar dismissed AaronBallman’s stale review via 4191d12 February 8, 2021 22:50

rdeodhar marked this pull request as ready for review February 8, 2021 22:50

rdeodhar requested a review from a team as a code owner February 8, 2021 22:50

rdeodhar requested a review from v-klochkov February 8, 2021 22:50

AaronBallman reviewed Feb 9, 2021

View reviewed changes

elizabethandrews reviewed Feb 9, 2021

View reviewed changes

clang/include/clang/Basic/DiagnosticSemaKinds.td Outdated Show resolved Hide resolved

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

elizabethandrews reviewed Feb 9, 2021

View reviewed changes

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

rdeodhar added 4 commits February 9, 2021 18:28

Modified tests to account for change from set to vector.

2ea7ac0

Formatting change.

d2afc19

Changed diagnostic.

e3f0280

Corrected test.

7bc5466

AaronBallman previously approved these changes Feb 10, 2021

View reviewed changes

Added clang AST test for attribute propagation.

1c4e611

rdeodhar dismissed AaronBallman’s stale review via 1c4e611 February 11, 2021 22:50

rdeodhar requested a review from elizabethandrews February 16, 2021 21:38

elizabethandrews reviewed Feb 17, 2021

View reviewed changes

rdeodhar added 3 commits February 17, 2021 15:04

Modified a test.

ae07883

Merge branch 'sycl' of https://github.com/intel/llvm into iwgo8

9e0216c

Changes for line numbers in modified sycl.hpp.

ec93452

rdeodhar requested a review from elizabethandrews February 18, 2021 16:16

elizabethandrews approved these changes Feb 18, 2021

View reviewed changes

v-klochkov approved these changes Mar 2, 2021

View reviewed changes

romanovvlad merged commit 496f9a0 into intel:sycl Mar 2, 2021

bader added a commit that referenced this pull request Mar 3, 2021

Revert "[SYCL] Copy attributes of parallel_for kernels to the wrapped…

92985c7

… versions when rounding up the range (#3154)" This reverts commit 496f9a0.

bader mentioned this pull request Mar 3, 2021

Revert "Copy attributes of parallel_for kernels to the wrapped versions when rounding up the range" #3293

Merged

bader added a commit that referenced this pull request Mar 3, 2021

Revert "[SYCL] Copy attributes of parallel_for kernels to the wrapped…

677a7ea

… versions when rounding up the range (#3154)" (#3293) This reverts commit 496f9a0.

rdeodhar mentioned this pull request Mar 4, 2021

Test change to accompany reverted commit 496f9a02fb1b1c0f0b56ddc7b8e5604be31a5f68 #3304

Closed

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

		template <typename Name, typename Type>

Copy attributes of parallel_for kernels to the wrapped versions when rounding up the range #3154

Copy attributes of parallel_for kernels to the wrapped versions when rounding up the range #3154

Uh oh!

Conversation

rdeodhar commented Feb 3, 2021

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!

Uh oh!

AaronBallman left a comment

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AaronBallman 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!

Choose a reason for hiding this comment

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

elizabethandrews 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!

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!