[SYCL] Add support for kernel name types templated using enums. #1675

elizabethandrews · 2020-05-12T06:04:38Z

Forward declaration of enum used as template parameter for kernel name type is now emitted in integration header.

Enumerators in name type is also replaced with their underlying integer value since the enum definition is not visible in integration header..

E.g. For following application/user code:

 enum class device : int {
     cpu,
     gpu
 };

 template<device DeviceType>
 class dummy_functor { };

The following is generated in integration header:

// Forward declaration of enum:
enum class device : int;
// Forward declaration of templated kernel function types: 
template <device DeviceType> class dummy_functor;
//Specializations of KernelInfo for kernel function types: 
template <> struct KernelInfo<dummy_functor<(device)1>> {..}

Signed-off-by: Elizabeth Andrews [email protected]

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews · 2020-05-12T06:07:11Z

@premanandrao I've added a test for anonymous namespaces. I'm waiting on your response about enums in SYCL headers, before removing the 'in cl namespace' check. Thanks!

elizabethandrews · 2020-05-12T06:14:21Z

@premanandrao I've added a test for anonymous namespaces. I'm waiting on your response about enums in SYCL headers, before removing the 'in cl namespace' check. Thanks!

I took another look at the integration header and the includes here are defines.hpp and kernel_desc.hpp. As far as I can tell the only namespaces visible in integration header are therefore 'cl' and anything declared in cstddef (which is included in kernel_desc.hpp). Given that information do you still think we should remove the 'enum defined in cl namespace' check? Removing this check would result in a lot of unnecessary forward declarations in the integration header - since most sycl programs use the enums defined in SYCL headers.

clang/lib/Sema/SemaSYCL.cpp

clang/test/CodeGenSYCL/int_header1.cpp

premanandrao · 2020-05-12T13:16:36Z

@premanandrao I've added a test for anonymous namespaces. I'm waiting on your response about enums in SYCL headers, before removing the 'in cl namespace' check. Thanks!

I took another look at the integration header and the includes here are defines.hpp and kernel_desc.hpp. As far as I can tell the only namespaces visible in integration header are therefore 'cl' and anything declared in cstddef (which is included in kernel_desc.hpp). Given that information do you still think we should remove the 'enum defined in cl namespace' check?

I don't think this is right. <kernel_desc.hpp> includes many other header files and a lot of other symbols including enums are visible. Perhaps you were looking at the minimal version in the test directories?

Removing this check would result in a lot of unnecessary forward declarations in the integration header - since most sycl programs use the enums defined in SYCL headers.

But the forward declarations are only for the cases where they are used as kernel name types.

clang/lib/Sema/SemaSYCL.cpp

elizabethandrews · 2020-05-12T14:15:24Z

But the forward declarations are only for the cases where they are used as kernel name types.

Oh right. Yes that's true. Got confused for a second. I'll remove the check.

elizabethandrews · 2020-05-13T14:41:32Z

I'm having some trouble with emitting the global scope specifiers. I can emit it for template arguments by getting Argument type for each argument and creating fullyQualifiedType

  QualType T = Arg.getAsType();
  QualType FullyQualifiedType = TypeName::getFullyQualifiedType(T, Ctx, true);
  ArgOS << FullyQualifiedType.getAsString(TypePolicy);

But I'm not sure what to do about template name. Currently I'm emitting it using printQualifiedName but this does not emit global scope specifier. Would someone know how I can go about this?

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2020-05-13T14:48:37Z

pecifiers. I can emit it for template arguments by getting Argument type for each argument an

What do you mean by "global scope specifier". Do you mean the scope resolution operator? (::)?

elizabethandrews · 2020-05-13T14:56:39Z

pecifiers. I can emit it for template arguments by getting Argument type for each argument an

What do you mean by "global scope specifier". Do you mean the scope resolution operator? (::)?

Yes. That is what I meant. After my patch,

     template <> struct KernelInfo<::nm1::KernelName3< ::nm1::nm2::KernelName0>>

has become

      template <> struct KernelInfo<nm1::KernelName3<nm1::nm2::KernelName0>>

5d22a8e was done a while ago to start the search for namespaces from global namespace

erichkeane · 2020-05-13T15:01:07Z

pecifiers. I can emit it for template arguments by getting Argument type for each argument an

What do you mean by "global scope specifier". Do you mean the scope resolution operator? (::)?

Yes. That is what I meant. After my patch,
     template <> struct KernelInfo<::nm1::KernelName3< ::nm1::nm2::KernelName0>>
has become
      template <> struct KernelInfo<nm1::KernelName3<nm1::nm2::KernelName0>>
5d22a8e was done a while ago to start the search for namespaces from global namespace

I have no idea how that worked (or why 'suppress typedefs' did it...).

I haven't looked closely into the printer, but I think @premanandrao has. Prem, can you take a look?

premanandrao · 2020-05-13T15:03:10Z

pecifiers. I can emit it for template arguments by getting Argument type for each argument an

What do you mean by "global scope specifier". Do you mean the scope resolution operator? (::)?

Yes. That is what I meant. After my patch,
     template <> struct KernelInfo<::nm1::KernelName3< ::nm1::nm2::KernelName0>>
has become
      template <> struct KernelInfo<nm1::KernelName3<nm1::nm2::KernelName0>>
5d22a8e was done a while ago to start the search for namespaces from global namespace
I have no idea how that worked (or why 'suppress typedefs' did it...).

I haven't looked closely into the printer, but I think @premanandrao has. Prem, can you take a look?

Yes, I will take a look.

Removed check for CL namespace. Emit scope resolution operator for global namespace. Added test when underlying type is not fixed. Signed-off-by: Elizabeth Andrews <[email protected]>

…o fixed underlying type Signed-off-by: Elizabeth Andrews <[email protected]>

clang/include/clang/AST/Decl.h

clang/lib/AST/Decl.cpp

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2020-05-14T13:59:06Z

clang/lib/Sema/SemaSYCL.cpp

+
+  for (unsigned I = 0; I < Args.size(); I++) {
+    const TemplateArgument &Arg = Args[I];
+    if (Arg.getKind() == TemplateArgument::ArgKind::Pack) {


This seems a lot like a few of the other places we emit arguments based on their kind. Is there a way to dispatch the body of much of this instead?

Maaaaybe. But I think the function body is not similar enough to make this efficient. Both functions are looping over arguments to emit 'something' but what is being emitted is different.

clang/lib/Sema/SemaSYCL.cpp

clang/test/CodeGenSYCL/int_header1.cpp

clang/test/CodeGenSYCL/kernelname-enum.cpp

Minor refactoring when emitting scope resolution operator. Added tests got unscoped enum. Added comment syntax for bool argument. Signed-off-by: Elizabeth Andrews <[email protected]>

Signed-off-by: Elizabeth Andrews <[email protected]>

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Elizabeth Andrews <[email protected]>

clang/lib/AST/Decl.cpp

elizabethandrews · 2020-05-14T23:54:01Z

Thanks for the review everyone!

clang/lib/AST/Decl.cpp

clang/test/CodeGenSYCL/kernelname-enum.cpp

bader · 2020-05-15T08:57:26Z

http://ci.llvm.intel.com:8010/#/builders/37/builds/859/steps/15/logs/FAIL__SYCL-Unit__CudaInteropGetNativeTests_interop - this failure seems to be unrelated to the patch.
This patch changes SYCL compiler, which has no effect on unittests.

Also fixed handling of template argument type - enum type. The prior patch handled only enum values, not the type itself. Signed-off-by: Elizabeth Andrews <[email protected]>

Signed-off-by: Elizabeth Andrews <[email protected]>

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/lib/Sema/SemaSYCL.cpp

Changed 'check' function to return true on error. Changed diagnostic message. Signed-off-by: Elizabeth Andrews <[email protected]>

clang/lib/Sema/SemaSYCL.cpp

clang/test/SemaSYCL/kernelname-enum.cpp

Fznamznon · 2020-05-18T08:14:25Z

clang/test/SemaSYCL/kernelname-enum.cpp

+// expected-error@+2 {{kernel name is invalid. Unscoped enum requires fixed underlying type}}
+template <unscoped_enum_no_type_set EnumType>


It would be great if this diagnostic will also point to concrete single_task/parallel_for kernel invocation call which caused this error. See

llvm/clang/test/SemaSYCL/unnamed-kernel.cpp

Line 51 in 9afcfad

// expected-error@+4 {{kernel needs to have a globally-visible name}}

for example.

This is because kernel location is set differently when kernel is defined using a lambda vs a functor. I just tested kernelname-enum.cpp using lambdas instead of functors and the diagnostic is generated at the point of kernel invocation like this example above. Since this is an existing issue with all diagnostics emitted when kernel is defined using functors, I'd like to work on this as a separate issue if that is alright with you.

Okay. We need to capture this problem somehow.

Ok. I'll file a bug and assign it to me.

clang/test/SemaSYCL/kernelname-enum.cpp

Signed-off-by: Elizabeth Andrews <[email protected]>

* sycl: (65 commits) [SYCL] Host task implementation (intel#1471) [SYCL] Update getting dependencies documentation (intel#1699) [SYCL] Fix types and transparent functors recognition in reduction (intel#1709) [SYCL][Doc] Get started guide clean-up (intel#1697) Add --spirv-fp-contract={on|off|fast} option (intel#509) [SYCL][Doc] Fix tbb target path in Get Started Guide. (intel#1695) [SYCL] Add support for kernel name types templated using enums. (intel#1675) [Driver][SYCL] Make -std=c++17 the default for DPC++ (intel#1662) AllocaInst should store Align instead of MaybeAlign. [X86] Replace selectScalarSSELoad ComplexPattern with PatFrags to handle the 3 types of loads we currently match. Harden IR and bitcode parsers against infinite size types. Revert "[nfc] test commit" [nfc] test commit Expose IRGen API to add the default IR attributes to a function definition. The release notes for ObjCBreakBeforeNestedBlockParam was placed between the release note for IndentCaseBlocks and its example code [VectorCombine] forward walk through instructions to improve chaining of transforms [PhaseOrdering] add vector reduction tests; NFC [InstCombine] Clean up alignment handling (NFC) [ARM] Patterns for VQSHRN [VectorCombine] add reduction-like patterns; NFC ...

elizabethandrews added 3 commits May 11, 2020 20:46

Add support for kernel name types templated using enums.

9715988

Signed-off-by: Elizabeth Andrews <[email protected]>

Fix crash of enum in TU scope. Add test

b337e43

Signed-off-by: Elizabeth Andrews <[email protected]>

Modify printing policy and add test for enums in anonymous namespace

c182c48

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews requested review from erichkeane and Fznamznon as code owners May 12, 2020 06:04

elizabethandrews requested a review from premanandrao May 12, 2020 06:04

Fznamznon reviewed May 12, 2020

View reviewed changes

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

clang/test/CodeGenSYCL/int_header1.cpp Outdated Show resolved Hide resolved

Fznamznon changed the title ~~Add support for kernel name types templated using enums.~~ [SYCL] Add support for kernel name types templated using enums. May 12, 2020

erichkeane reviewed May 12, 2020

View reviewed changes

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

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

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

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

elizabethandrews commented May 13, 2020

View reviewed changes

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

elizabethandrews added 2 commits May 14, 2020 06:32

Implemented review comments:

f6b9b59

Removed check for CL namespace. Emit scope resolution operator for global namespace. Added test when underlying type is not fixed. Signed-off-by: Elizabeth Andrews <[email protected]>

Add back up promotion type to handle enum forward declarations with n…

07135ab

…o fixed underlying type Signed-off-by: Elizabeth Andrews <[email protected]>

erichkeane reviewed May 14, 2020

View reviewed changes

premanandrao reviewed May 14, 2020

View reviewed changes

clang/test/CodeGenSYCL/int_header1.cpp Show resolved Hide resolved

Naghasan reviewed May 14, 2020

View reviewed changes

clang/test/CodeGenSYCL/kernelname-enum.cpp Show resolved Hide resolved

elizabethandrews added 2 commits May 14, 2020 11:20

Implemented review comments including:

df0994a

Minor refactoring when emitting scope resolution operator. Added tests got unscoped enum. Added comment syntax for bool argument. Signed-off-by: Elizabeth Andrews <[email protected]>

Refactored printArguments

46ef79d

Signed-off-by: Elizabeth Andrews <[email protected]>

erichkeane reviewed May 14, 2020

View reviewed changes

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

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

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

elizabethandrews added 3 commits May 14, 2020 16:26

Minor refactoring

6262d69

Signed-off-by: Elizabeth Andrews <[email protected]>

Missed a line in previous commit

b59e1a3

Signed-off-by: Elizabeth Andrews <[email protected]>

Clang-Format Changes

0a83e16

Signed-off-by: Elizabeth Andrews <[email protected]>

erichkeane previously approved these changes May 14, 2020

View reviewed changes

clang/lib/AST/Decl.cpp Show resolved Hide resolved

Fznamznon previously approved these changes May 15, 2020

View reviewed changes

clang/lib/AST/Decl.cpp Outdated Show resolved Hide resolved

Naghasan reviewed May 15, 2020

View reviewed changes

clang/test/CodeGenSYCL/kernelname-enum.cpp Outdated Show resolved Hide resolved

This was referenced May 15, 2020

[SYCL] Enable PI unit testing on multiple plugins. #1647

Merged

[SYCL] Enable PI unit testing on multiple plugins. #1694

Merged

Added diagnostic to handle unscoped enums with no fixed underlying type.

52add49

Also fixed handling of template argument type - enum type. The prior patch handled only enum values, not the type itself. Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews dismissed stale reviews from Fznamznon and erichkeane via 52add49 May 16, 2020 03:48

Clang Format changes

9d07023

Signed-off-by: Elizabeth Andrews <[email protected]>

erichkeane reviewed May 16, 2020

View reviewed changes

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

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

Implemented review comments:

eead667

Changed 'check' function to return true on error. Changed diagnostic message. Signed-off-by: Elizabeth Andrews <[email protected]>

erichkeane previously approved these changes May 17, 2020

View reviewed changes

Fznamznon reviewed May 18, 2020

View reviewed changes

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

Fznamznon reviewed May 18, 2020

View reviewed changes

clang/test/SemaSYCL/kernelname-enum.cpp Outdated Show resolved Hide resolved

Fznamznon reviewed May 18, 2020

View reviewed changes

Removed commented code

1064c45

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews dismissed erichkeane’s stale review via 1064c45 May 18, 2020 16:56

premanandrao approved these changes May 18, 2020

View reviewed changes

erichkeane approved these changes May 18, 2020

View reviewed changes

Fznamznon approved these changes May 18, 2020

View reviewed changes

bader merged commit e7020a1 into intel:sycl May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Add support for kernel name types templated using enums. #1675

[SYCL] Add support for kernel name types templated using enums. #1675

elizabethandrews commented May 12, 2020

elizabethandrews commented May 12, 2020

elizabethandrews commented May 12, 2020

premanandrao commented May 12, 2020

elizabethandrews commented May 12, 2020

elizabethandrews commented May 13, 2020

erichkeane commented May 13, 2020

elizabethandrews commented May 13, 2020 •

edited

Loading

erichkeane commented May 13, 2020

premanandrao commented May 13, 2020

erichkeane May 14, 2020

elizabethandrews May 14, 2020

elizabethandrews commented May 14, 2020

bader commented May 15, 2020

Fznamznon May 18, 2020

elizabethandrews May 18, 2020

Fznamznon May 18, 2020

elizabethandrews May 18, 2020

elizabethandrews May 18, 2020

		// expected-error@+2 {{kernel name is invalid. Unscoped enum requires fixed underlying type}}
		template <unscoped_enum_no_type_set EnumType>

[SYCL] Add support for kernel name types templated using enums. #1675

[SYCL] Add support for kernel name types templated using enums. #1675

Conversation

elizabethandrews commented May 12, 2020

elizabethandrews commented May 12, 2020

elizabethandrews commented May 12, 2020

premanandrao commented May 12, 2020

elizabethandrews commented May 12, 2020

elizabethandrews commented May 13, 2020

erichkeane commented May 13, 2020

elizabethandrews commented May 13, 2020 • edited Loading

erichkeane commented May 13, 2020

premanandrao commented May 13, 2020

erichkeane May 14, 2020

Choose a reason for hiding this comment

elizabethandrews May 14, 2020

Choose a reason for hiding this comment

elizabethandrews commented May 14, 2020

bader commented May 15, 2020

Fznamznon May 18, 2020

Choose a reason for hiding this comment

elizabethandrews May 18, 2020

Choose a reason for hiding this comment

Fznamznon May 18, 2020

Choose a reason for hiding this comment

elizabethandrews May 18, 2020

Choose a reason for hiding this comment

elizabethandrews May 18, 2020

Choose a reason for hiding this comment

elizabethandrews commented May 13, 2020 •

edited

Loading