From 22d92a942cff9e5d0e7d6a643de178a8129e7eca Mon Sep 17 00:00:00 2001 From: Elizabeth Andrews Date: Thu, 10 Feb 2022 19:00:49 -0800 Subject: [PATCH 1/4] [SYCL] Fix SYCL Kernel Body Check SYCLKernelAttr can be used to check if a given function is a SYCL kernel body function or not. A SYCL kernel body function is the operator() method of a SYCL Kernel Functor or Lamda Function. The existing check which only tested if a FunctionDecl was an operator or not, allowed for false positives. This patch adds an additional check for th SYCLKernelAttr, which will be attached to the kernel body function's parent/ parent's parent (for wrapped kernels) Signed-off-by: Elizabeth Andrews --- clang/lib/Sema/SemaSYCL.cpp | 63 +++++++++++----------- clang/test/SemaSYCL/non-kernel-functor.cpp | 21 ++++++++ 2 files changed, 53 insertions(+), 31 deletions(-) create mode 100644 clang/test/SemaSYCL/non-kernel-functor.cpp diff --git a/clang/lib/Sema/SemaSYCL.cpp b/clang/lib/Sema/SemaSYCL.cpp index 226c72b3b93be..e16ef27a4f68d 100644 --- a/clang/lib/Sema/SemaSYCL.cpp +++ b/clang/lib/Sema/SemaSYCL.cpp @@ -503,12 +503,16 @@ void Sema::checkSYCLDeviceVarDecl(VarDecl *Var) { checkSYCLType(*this, Ty, Loc, Visited); } -// Tests whether given function is a lambda function or '()' operator used as -// SYCL kernel body function (e.g. in parallel_for). +// Tests whether OperatorFD is a lambda function or '()' operator used as +// SYCL kernel body function (e.g. in parallel_for). If OperatorFD is a SYCL +// kernel body function, it's parent (or parent's parent for wrapped kernel), +// will have be marked with SYCLKernelAttr. // NOTE: This is incomplete implemenation. See TODO in the FE TODO list for the // ESIMD extension. -static bool isSYCLKernelBodyFunction(FunctionDecl *FD) { - return FD->getOverloadedOperator() == OO_Call; +static bool isSYCLKernelBodyFunction(FunctionDecl *OperatorFD, + FunctionDecl *KernelFD) { + return OperatorFD->getOverloadedOperator() == OO_Call && + KernelFD->hasAttr(); } static bool isSYCLUndefinedAllowed(const FunctionDecl *Callee, @@ -818,32 +822,29 @@ class SingleDeviceFunctionTracker { DirectlyCalled); } - // Calculate the kernel body. Note the 'isSYCLKernelBodyFunction' only - // tests that it is operator(), so hopefully this doesn't get us too many - // false-positives. - if (isSYCLKernelBodyFunction(CurrentDecl)) { - // This is a direct callee of the kernel. - if (CallStack.size() == 1) { - assert(!KernelBody && "inconsistent call graph - only one kernel body " - "function can be called"); - KernelBody = CurrentDecl; - } else if (CallStack.size() == 2 && KernelBody == CallStack.back()) { - // To implement rounding-up of a parallel-for range the - // SYCL header implementation modifies the kernel call like this: - // auto Wrapper = [=](TransformedArgType Arg) { - // if (Arg[0] >= NumWorkItems[0]) - // return; - // Arg.set_allowed_range(NumWorkItems); - // KernelFunc(Arg); - // }; - // - // This transformation leads to a condition where a kernel body - // function becomes callable from a new kernel body function. - // Hence this test. - // FIXME: We need to be more selective here, this can be hit by simply - // having a kernel lambda with a lambda call inside of it. - KernelBody = CurrentDecl; - } + // Calculate the kernel body. Since Kernel body is a direct callee of the + // kernel, or a wrapped inside a parallel-for, size of CallStack will be + // either 1 or 2. + if (CallStack.size() == 1 && + isSYCLKernelBodyFunction(CurrentDecl, CallStack.front())) { + assert(!KernelBody && "inconsistent call graph - only one kernel body " + "function can be called"); + KernelBody = CurrentDecl; + } else if (CallStack.size() == 2 && + isSYCLKernelBodyFunction(CurrentDecl, CallStack.front())) { + // To implement rounding-up of a parallel-for range the + // SYCL header implementation modifies the kernel call like this: + // auto Wrapper = [=](TransformedArgType Arg) { + // if (Arg[0] >= NumWorkItems[0]) + // return; + // Arg.set_allowed_range(NumWorkItems); + // KernelFunc(Arg); + // }; + // + // This transformation leads to a condition where a kernel body + // function becomes callable from a new kernel body function. + // Hence this test. + KernelBody = CurrentDecl; } // Recurse. @@ -3615,7 +3616,7 @@ void Sema::copySYCLKernelAttrs(const CXXRecordDecl *KernelObj) { FunctionDecl *FD = WorkList.back().first; FunctionDecl *ParentFD = WorkList.back().second; - if ((ParentFD == OpParens) && isSYCLKernelBodyFunction(FD)) { + if ((ParentFD == OpParens) && (FD->getOverloadedOperator() == OO_Call)) { KernelBody = FD; break; } diff --git a/clang/test/SemaSYCL/non-kernel-functor.cpp b/clang/test/SemaSYCL/non-kernel-functor.cpp new file mode 100644 index 0000000000000..8a0c9a0b571a0 --- /dev/null +++ b/clang/test/SemaSYCL/non-kernel-functor.cpp @@ -0,0 +1,21 @@ +// RUN: %clang_cc1 -fsycl-is-device -triple spir64-unknown-unknown -verify %s + +// Test to verify that non-kernel functors are not processed as SYCL kernel +// functors + +// expected-no-diagnostics +class First { +public: + void operator()() { return; } +}; + +class Second { +public: + First operator()() { return First(); } +}; + +SYCL_EXTERNAL +void foo() { + Second m_uold; + m_uold()(); +} From 79bf8d4e88d6fed54f7298b490b04d24f30742eb Mon Sep 17 00:00:00 2001 From: Elizabeth Andrews Date: Thu, 10 Feb 2022 19:25:09 -0800 Subject: [PATCH 2/4] Correct comment and rename variable Signed-off-by: Elizabeth Andrews --- clang/lib/Sema/SemaSYCL.cpp | 2 +- clang/test/SemaSYCL/non-kernel-functor.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/lib/Sema/SemaSYCL.cpp b/clang/lib/Sema/SemaSYCL.cpp index e16ef27a4f68d..7df09816650fe 100644 --- a/clang/lib/Sema/SemaSYCL.cpp +++ b/clang/lib/Sema/SemaSYCL.cpp @@ -506,7 +506,7 @@ void Sema::checkSYCLDeviceVarDecl(VarDecl *Var) { // Tests whether OperatorFD is a lambda function or '()' operator used as // SYCL kernel body function (e.g. in parallel_for). If OperatorFD is a SYCL // kernel body function, it's parent (or parent's parent for wrapped kernel), -// will have be marked with SYCLKernelAttr. +// will be marked with SYCLKernelAttr. // NOTE: This is incomplete implemenation. See TODO in the FE TODO list for the // ESIMD extension. static bool isSYCLKernelBodyFunction(FunctionDecl *OperatorFD, diff --git a/clang/test/SemaSYCL/non-kernel-functor.cpp b/clang/test/SemaSYCL/non-kernel-functor.cpp index 8a0c9a0b571a0..e8a93c5c243ab 100644 --- a/clang/test/SemaSYCL/non-kernel-functor.cpp +++ b/clang/test/SemaSYCL/non-kernel-functor.cpp @@ -16,6 +16,6 @@ class Second { SYCL_EXTERNAL void foo() { - Second m_uold; - m_uold()(); + Second NonKernelFunctorObj; + NonKernelFunctorObj()(); } From 2eaf47f404156c9d93438d790709daa1891cef94 Mon Sep 17 00:00:00 2001 From: Elizabeth Andrews Date: Fri, 11 Feb 2022 08:27:24 -0800 Subject: [PATCH 3/4] Revert changes and add check for attribute Signed-off-by: Elizabeth Andrews --- clang/lib/Sema/SemaSYCL.cpp | 64 ++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/clang/lib/Sema/SemaSYCL.cpp b/clang/lib/Sema/SemaSYCL.cpp index 7df09816650fe..38e50fc3a36d3 100644 --- a/clang/lib/Sema/SemaSYCL.cpp +++ b/clang/lib/Sema/SemaSYCL.cpp @@ -503,16 +503,12 @@ void Sema::checkSYCLDeviceVarDecl(VarDecl *Var) { checkSYCLType(*this, Ty, Loc, Visited); } -// Tests whether OperatorFD is a lambda function or '()' operator used as -// SYCL kernel body function (e.g. in parallel_for). If OperatorFD is a SYCL -// kernel body function, it's parent (or parent's parent for wrapped kernel), -// will be marked with SYCLKernelAttr. +// Tests whether given function is a lambda function or '()' operator used as +// SYCL kernel body function (e.g. in parallel_for). // NOTE: This is incomplete implemenation. See TODO in the FE TODO list for the // ESIMD extension. -static bool isSYCLKernelBodyFunction(FunctionDecl *OperatorFD, - FunctionDecl *KernelFD) { - return OperatorFD->getOverloadedOperator() == OO_Call && - KernelFD->hasAttr(); +static bool isSYCLKernelBodyFunction(FunctionDecl *FD) { + return FD->getOverloadedOperator() == OO_Call; } static bool isSYCLUndefinedAllowed(const FunctionDecl *Callee, @@ -822,29 +818,33 @@ class SingleDeviceFunctionTracker { DirectlyCalled); } - // Calculate the kernel body. Since Kernel body is a direct callee of the - // kernel, or a wrapped inside a parallel-for, size of CallStack will be - // either 1 or 2. - if (CallStack.size() == 1 && - isSYCLKernelBodyFunction(CurrentDecl, CallStack.front())) { - assert(!KernelBody && "inconsistent call graph - only one kernel body " - "function can be called"); - KernelBody = CurrentDecl; - } else if (CallStack.size() == 2 && - isSYCLKernelBodyFunction(CurrentDecl, CallStack.front())) { - // To implement rounding-up of a parallel-for range the - // SYCL header implementation modifies the kernel call like this: - // auto Wrapper = [=](TransformedArgType Arg) { - // if (Arg[0] >= NumWorkItems[0]) - // return; - // Arg.set_allowed_range(NumWorkItems); - // KernelFunc(Arg); - // }; - // - // This transformation leads to a condition where a kernel body - // function becomes callable from a new kernel body function. - // Hence this test. - KernelBody = CurrentDecl; + // Calculate the kernel body. Note the 'isSYCLKernelBodyFunction' only + // tests that it is operator(), so hopefully this doesn't get us too many + // false-positives. + if (isSYCLKernelBodyFunction(CurrentDecl)) { + // This is a direct callee of the kernel. + if (CallStack.size() == 1 && + CallStack.back()->hasAttr()) { + assert(!KernelBody && "inconsistent call graph - only one kernel body " + "function can be called"); + KernelBody = CurrentDecl; + } else if (CallStack.size() == 2 && KernelBody == CallStack.back()) { + // To implement rounding-up of a parallel-for range the + // SYCL header implementation modifies the kernel call like this: + // auto Wrapper = [=](TransformedArgType Arg) { + // if (Arg[0] >= NumWorkItems[0]) + // return; + // Arg.set_allowed_range(NumWorkItems); + // KernelFunc(Arg); + // }; + // + // This transformation leads to a condition where a kernel body + // function becomes callable from a new kernel body function. + // Hence this test. + // FIXME: We need to be more selective here, this can be hit by simply + // having a kernel lambda with a lambda call inside of it. + KernelBody = CurrentDecl; + } } // Recurse. @@ -3616,7 +3616,7 @@ void Sema::copySYCLKernelAttrs(const CXXRecordDecl *KernelObj) { FunctionDecl *FD = WorkList.back().first; FunctionDecl *ParentFD = WorkList.back().second; - if ((ParentFD == OpParens) && (FD->getOverloadedOperator() == OO_Call)) { + if ((ParentFD == OpParens) && isSYCLKernelBodyFunction(FD)) { KernelBody = FD; break; } From c523c9b109d6ed8813e62fbc1e02d95951f8d9c6 Mon Sep 17 00:00:00 2001 From: Elizabeth Andrews Date: Fri, 11 Feb 2022 08:38:50 -0800 Subject: [PATCH 4/4] Remove extra space Signed-off-by: Elizabeth Andrews --- clang/test/SemaSYCL/non-kernel-functor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/test/SemaSYCL/non-kernel-functor.cpp b/clang/test/SemaSYCL/non-kernel-functor.cpp index e8a93c5c243ab..f321d57dfded6 100644 --- a/clang/test/SemaSYCL/non-kernel-functor.cpp +++ b/clang/test/SemaSYCL/non-kernel-functor.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -fsycl-is-device -triple spir64-unknown-unknown -verify %s +// RUN: %clang_cc1 -fsycl-is-device -triple spir64-unknown-unknown -verify %s // Test to verify that non-kernel functors are not processed as SYCL kernel // functors