[SYCL] Support or diagnose use of namespace std types as kernel type names #1963

jtmott-intel · 2020-06-23T22:43:59Z

This is a continuation of #1579. Sorry I couldn't commit to the branch named in that PR.

This PR has premanandrao's original commit plus one of mine to address the comments in 1579 -- specifically this conversation.

I removed the changes to the generated header and added a diagnostic to make nullptr_t in kernel names an error. It's possible to make nullptr_t work like int works, but sounds like we don't want to encourage using builtin types as kernel names. The diagnostic message is open to wordsmithing.

…names When std::nullptr_t is used as a kernel type, the generated integration header uses 'nullptr_t'. This causes lookup errors. Use 'std::nullptr_t' instead. std::max_align_t is defined (in one implementation) as a typedef of an anonymous struct. This causes errors when attempting to forward declare the type in the integration header. Diagnose such cases earlier. Signed-off-by: Premanand M Rao <premanand.m.rao@intel.com>

# Conflicts: # clang/include/clang/Basic/DiagnosticSemaKinds.td

Fznamznon · 2020-06-24T13:24:58Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -10835,6 +10835,7 @@ def err_sycl_kernel_incorrectly_named : Error<
  "kernel %select{name is missing"
  "|needs to have a globally-visible name"
  "|name is invalid. Unscoped enum requires fixed underlying type"
+  "|name cannot be or use std::nullptr_t"


Just wondering, why we disallow only nullptr_t?
Your PR message says " sounds like we don't want to encourage using builtin types as kernel names. "

nullptr_t is special because builtin type names are (typically) accessible without a header, but nullptr_t is a builtin type that isn't known by any name until we include a header (or forward declare). The "don't want to encourage using builtins" comes from rolandschulz's comment -- "Adding extra support for std types is not needed," and, "there is no promise that std types work as lambda name." But I may have misinterpreted his comments, and others asking him for clarification.

Fznamznon · 2020-06-24T13:28:09Z

clang/lib/Sema/SemaSYCL.cpp

+        Diag.Report(KernelLocation, diag::err_sycl_kernel_incorrectly_named)
+            << /* kernel name is missing */ 0;


I spend some time looking into these changes, and I don't get why we didn't start emitting "kernel name is missing" on each unnamed lambda, because before your changes this diagnostic was under if (!UnnamedLambdaSupport) and it looked correct.

The original "if" was if (TD && !UnnamedLambdaSupport), and when this evaluates false, then the else block would still use TD anyway -- which seems weird. premanandrao's commit refactored the TD check out to if (!TD) break;

I'm still a bit confused about this change. Can you give an example where if (TD) evaluates true and KernelNameIsMissing i.e. TD does not have a name?

Also, this change is not technically required for 'std' namespace check right? Or am I missing something?

I think this happens when it is invalid.
It is not needed strictly for the specific 'std' namespace check, but it occurs in those contexts too.

Normally the if (TD) check would ensure that a kernel name type is a TagDecl -- that is, one of a struct/union/class/enum -- which would catch kernel name types such as int. I say "normally", though, because earlier in the call stack, we also ensure that a kernel name type is a CXXRecordDecl -- that is, one of a struct/union/class. Which means, sadly, that the if (TD) part may actually be dead code. But, it was dead code even before, and independent of, this PR. I had considered doing a clean-up of the existing code, but... I feel like this PR has already exceed its side-quest quota. :-)

Yes this is part of what's being checked in, but the original had dead code too, and this part just moves the dead code a couple lines over.

I see. In that case, I don't mind this being addressed in a separate PR.

OK then sounds like we're opting to not revert the April refactoring? And this PR would stay as is?

Seems like folks still aren't comfortable with those changes, and since they come from an older time in this PR and don't seem to be needed anymore, I pushed a commit to revert that bit of refactoring. Buildbot is running. I'll ping folks when ready for re-review approvals.

The changes @elizabethandrews was concerned about are reverted. All checks passed. Need review approvals again. @premanandrao @rbegam

erichkeane · 2020-06-24T17:48:16Z

I disagree with this approach. While Roland's comment is 'Because there is no promise that std types work as lambda name', that only applies as the top level item. a kernel named

<std::string>

SHOULD be illegal, if we're going to implement Roland's comment, we should cover EVERY type in std.

However,

<SomeTemplate<std::string>>
or
<SomeTemplate<std::nullptr_t>>

the std type is NOT the kernel name, an instance of SomeTemplate is. We should have to support this case.

I believe we'd discussed before that we should just serialize std::nullptr_t as decltype(nullptr) to make this work.

premanandrao · 2020-06-24T19:14:04Z

I disagree with this approach. While Roland's comment is 'Because there is no promise that std types work as lambda name', that only applies as the top level item. a kernel named
<std::string>
SHOULD be illegal, if we're going to implement Roland's comment, we should cover EVERY type in std.

However,
<SomeTemplate<std::string>>
or
<SomeTemplate<std::nullptr_t>>
the std type is NOT the kernel name, an instance of SomeTemplate is. We should have to support this case.

I believe we'd discussed before that we should just serialize std::nullptr_t as decltype(nullptr) to make this work.

Agreed with the second part - only top-level should be disallowed.
About the first part - did Roland mean 'all' std types or just the fundamental ones like size_t, ptrdiff_t etc.?

erichkeane · 2020-06-24T19:19:01Z

I disagree with this approach. While Roland's comment is 'Because there is no promise that std types work as lambda name', that only applies as the top level item. a kernel named
<std::string>
SHOULD be illegal, if we're going to implement Roland's comment, we should cover EVERY type in std.
However,
<SomeTemplate<std::string>>
or
<SomeTemplate<std::nullptr_t>>
the std type is NOT the kernel name, an instance of SomeTemplate is. We should have to support this case.
I believe we'd discussed before that we should just serialize std::nullptr_t as decltype(nullptr) to make this work.
Agreed with the second part - only top-level should be disallowed.
About the first part - did Roland mean 'all' std types or just the fundamental ones like size_t, ptrdiff_t etc.?

I read it as 'all'... @rolandschulz can you clarify?

jtmott-intel · 2020-06-24T20:46:52Z

While we wait for clarification, just adding a couple notes:

If we use the decltype(nullptr) trick, then std::nullptr_t would work for *both* the top-level kernel name and as a template parameter.
Some types may never work, not even as template parameters, such as <SomeTemplate<std::max_align_t>>, because max_align_t is an anonymous struct. The issue isn't whether it's the top-level kernel name or template parameter. The issue is can we write it out as textual source to the integration header, and that complication isn't limited to std types.

rolandschulz · 2020-06-24T21:27:22Z

Yes I meant all std types. Only types which can be forward declared have to be supported and it isn't valid to forward declare std types.

erichkeane · 2020-06-24T21:34:42Z

* Some types may never work, not even as template parameters, such as `<SomeTemplate<std::max_align_t>>`, because max_align_t is an anonymous struct. The issue isn't whether it's the top-level kernel name or template parameter. The issue is can we write it out as textual source to the integration header, and that complication isn't limited to std types.

This point here is particularly annoying... @rolandschulz : what clarification to the SYCL spec needs to me made to allow us to prevent ANY use of namespace-std in kernel names? Or do you and @mkinsner / @jbrodman /etc already feel that is the case?

rolandschulz · 2020-06-25T01:10:15Z

For the implementation we should just assume that it's OK that nothing in std is allowed. This is clearly the intention. If someone thinks this needs to be clarified in the spec even for 1.2.1 not just upcoming SYCL we can do that.

jtmott-intel · 2020-06-25T02:50:37Z

One of the relevant use cases seems to be a kernel inside a function template, because the function template parameters get reused in the kernel name to keep each name type unique. We do this ourselves in the SYCL handler "fill" function template. The value type to fill is a parameter that gets reused in the kernel name. If we call fill with a std::complex number, then if the kernel rejects it, fill rejects it.

jtmott-intel · 2020-07-02T20:48:59Z

Latest commit disallows names from std namespace. std::nullptr_t is still special cased since it resolves to a builtin instead of a decl.

# Conflicts: # sycl/test/sub_group/reduce.cpp # sycl/test/sub_group/scan.cpp

bader · 2020-07-15T07:58:15Z

@jtmott-intel, please, resolve CI failures.

jtmott-intel · 2020-07-16T07:08:37Z

All checks have passed.

Here's a quick re-summary.

Originally we were going to emit an error diagnostic for kernel names with std::nullptr_t. That type was never guaranteed to work as a kernel name, and indeed the integration header wasn't generating correctly for that type.

Later we decided to disallow all std types, since the standard doesn't allow us to forward declare anything in std. If ever a std type appeared to work in a kernel name, it's because the dragons were merciful.

After implementing this change, several tests broke because they were (indirectly) using std types in their kernel names. Most of those occurrences were a kernel inside a template, because the template needs different kernel names for each generated specialization, and when we re-purposed the template's parameters for the kernel name, then any std type passed to the template also ended up in the name.

// example, before this PR
template<typename T> void test() {
    ...
    cgh.parallel_for<KernelName<T>>( ...
    ...
}
test<std::nullptr_t>();       // bad, std::nullptr_t indirectly used in kernel name
test<std::complex<double>>(); // bad, std::complex indirectly used in kernel name

I updated the impacted tests and made the kernel name its own distinct template parameter so we wouldn't need to re-purpose the other parameters anymore.

// example, after this PR
template<typename SpecializationKernelName, typename T> void test() {
    ...
    cgh.parallel_for<SpecializationKernelName>( ...
    ...
}
test<class SpecializationKernelName_ABC, std::nullptr_t>();       // ok, kernel name and std::nullptr_t are separate
test<class SpecializationKernelName_XYZ, std::complex<double>>(); // ok, kernel name and std::complex are separate

This does, however, put more responsibility on the user to pass unique kernel names. If they don't, then existing diagnostics will already tell them.

bader · 2020-07-16T08:51:21Z

This is a continuation of #1579. Sorry I couldn't commit to the branch named in that PR.

Please, close #1579 if you are not going to merge it.

erichkeane · 2020-08-18T20:16:19Z

Lambda names are required in SYCL 1.2.1 but not in DPC++ or SYCL 2020 provisional. For the later the extension should be enabled by default. Also note that SYCL 1.2.1 doesn't require diagnostic for missing lambda names. Therefore in theory we could always enable the extension. But it would be good to have a warning for the users in SYCL 1.2.1 mode for them to know this isn't portable if they don't provide a lambda name. But such a diagnostic could be done independent of our implementation using the string representation (as we do for the extension) rather than the type (as we do without the extension).

I see, thanks for the update! So we COULD still enable unnamed lambdas in SYCL 2020 mode.

That said, we should still have the prohibition on std types in 1.2.1 mode. I believe the enabling unnamed lambdas in 2020 mode is orthogonal to this patch, so I'd think we want to do that as a separate patch.

# Conflicts: # clang/test/SemaSYCL/unnamed-kernel.cpp # sycl/test/group-algorithm/exclusive_scan.cpp # sycl/test/group-algorithm/inclusive_scan.cpp # sycl/test/group-algorithm/reduce.cpp # sycl/test/reduction/reduction_ctor.cpp # sycl/test/reduction/reduction_nd_conditional.cpp # sycl/test/reduction/reduction_nd_ext_type.hpp # sycl/test/reduction/reduction_nd_s0_dw.cpp # sycl/test/reduction/reduction_nd_s0_rw.cpp # sycl/test/reduction/reduction_nd_s1_dw.cpp # sycl/test/reduction/reduction_nd_s1_rw.cpp # sycl/test/reduction/reduction_placeholder.cpp # sycl/test/reduction/reduction_transparent.cpp # sycl/test/reduction/reduction_usm.cpp # sycl/test/sub_group/generic-shuffle.cpp # sycl/test/sub_group/reduce.hpp # sycl/test/sub_group/scan.hpp

jtmott-intel · 2020-08-31T23:21:12Z

Hi, @bader . Any chance this PR is having a problem with the Windows buildbots? There's no significant code changes since the last "all checks passed". The specific tests that fail can be different on each run -- sometimes it's reduction/reduction_transparent.cpp, sometimes it's host-interop-task/interop-task.cpp -- but it's always Windows-only. And I haven't been able to reproduce the failures locally.

romanovvlad · 2020-09-01T09:29:11Z

Hi, @bader . Any chance this PR is having a problem with the Windows buildbots? There's no significant code changes since the last "all checks passed". The specific tests that fail can be different on each run -- sometimes it's reduction/reduction_transparent.cpp, sometimes it's host-interop-task/interop-task.cpp -- but it's always Windows-only. And I haven't been able to reproduce the failures locally.

The test is flaky, we disabled it on windows. Could you please merge your branch with the latest sycl branch?

jtmott-intel · 2020-09-01T19:18:28Z

@romanovvlad After the latest merge from sycl, I'm getting a Windows-only error for sycl/test/reduction/reduction_transparent.cpp and reduction_usm.cpp. They're returning exit code 3221226505 and no output on stdout or stderr. I'm not familiar with that exit code but googling suggests it's a generic "Internal process error" coming from Windows. I can keep investigating but I wanted to check if this a buildbot issue?

romanovvlad · 2020-09-02T12:28:21Z

@romanovvlad After the latest merge from sycl, I'm getting a Windows-only error for sycl/test/reduction/reduction_transparent.cpp and reduction_usm.cpp. They're returning exit code 3221226505 and no output on stdout or stderr. I'm not familiar with that exit code but googling suggests it's a generic "Internal process error" coming from Windows. I can keep investigating but I wanted to check if this a buildbot issue?

I have not seen this fails before. @vladimirlaz @tfzhu have you seen this issue in other PRs?
@jtmott-intel Are you sure that this issue is not introduced by your patch?

jtmott-intel · 2020-09-02T17:17:35Z

@romanovvlad This PR does touch the two failing tests, but I also got an "all checks passed" earlier, and since then I've only merged from sycl branch. I've looked over the changes to those files character by character, and everything still looks just as right as when all checks passed. None of the changes are OS-specific. And I've tried to reproduce the failure locally but can't reproduce.

romanovvlad · 2020-09-03T12:12:28Z

@romanovvlad This PR does touch the two failing tests, but I also got an "all checks passed" earlier, and since then I've only merged from sycl branch. I've looked over the changes to those files character by character, and everything still looks just as right as when all checks passed. None of the changes are OS-specific. And I've tried to reproduce the failure locally but can't reproduce.

I've checked several other failing pre-commits on windows and I do not see these test failing.
Your patch may introduce sporadic failure.

jtmott-intel · 2020-09-05T06:06:24Z

All checks passed! Sorry, folks. Yes this latest problem was my fault. I made a mistake in a merge resolution and used the same kernel name twice. I didn't feel good about using a PR to troubleshoot the issue, but I couldn't reproduce it any other way, so I finally used a separate dummy PR to narrow down where the problem was coming from, and -- I'm happy to say it's fixed now. Final approval and merge may commence. I assume folks will prefer a squash merge for this one? :-)

@romanovvlad @elizabethandrews @premanandrao

premanandrao

@jtmott-intel, I don't see any major changes since I last reviewed this. The set of changes look good to me.

@elizabethandrews, @Fznamznon, okay with you?

jtmott-intel · 2020-09-10T19:07:33Z

I see @premanandrao 's approval, but github now appears to be blocking because it's waiting on an approval from someone in the intel/llvm-reviewers-runtime team. @romanovvlad @rbegam ?

rbegam

LGTM

…el type names" This reverts commit 46c54c6. # Conflicts: # clang/test/CodeGenSYCL/stdtypes_kernel_type.cpp # clang/test/SemaSYCL/unnamed-kernel.cpp

elizabethandrews

LGTM. Thanks!

Can you file a GitHub issue to look into the dead code so we don't forget about it?

jtmott-intel · 2020-09-12T01:15:36Z

The base branch restricts merging to authorized users -- and apparently I'm not an authorized user. :-) Maybe @premanandrao or @elizabethandrews can one of you click the merge button for me?

elizabethandrews · 2020-09-14T13:57:04Z

The base branch restricts merging to authorized users -- and apparently I'm not an authorized user. :-) Maybe @premanandrao or @elizabethandrews can one of you click the merge button for me?

We're not authorized to either :) I see @bader already merged your commit but FYI, @bader and @pvchupin have merge rights.

premanandrao and others added 3 commits April 23, 2020 12:23

Emit diagnostic when kernel names use nullptr_t

5e8004d

Merge from branch sycl

26b38f9

# Conflicts: # clang/include/clang/Basic/DiagnosticSemaKinds.td

jtmott-intel requested review from rolandschulz, bader, erichkeane, Fznamznon, kbobrovs and premanandrao June 23, 2020 22:43

jtmott-intel requested a review from elizabethandrews as a code owner June 23, 2020 22:43

Fznamznon reviewed Jun 24, 2020

View reviewed changes

Disallow std names in kernel names

c21b9df

jtmott-intel added 2 commits July 2, 2020 14:04

Clang format patch

1d345ec

Fixing tests, specifically tests with std kernels

d2d55b0

jtmott-intel requested a review from a team as a code owner July 10, 2020 17:48

jtmott-intel requested a review from rbegam July 10, 2020 17:48

Merge branch 'sycl'

e678857

# Conflicts: # sycl/test/sub_group/reduce.cpp # sycl/test/sub_group/scan.cpp

Fixing tests, specifically tests with std kernels

e6afe60

jtmott-intel added 2 commits August 20, 2020 19:40

Clang format and update expected error line numbers

04bd440

Merge branch sycl

736118f

Fix bad merge resolution

a96b556

premanandrao previously approved these changes Sep 10, 2020

View reviewed changes

rbegam previously approved these changes Sep 10, 2020

View reviewed changes

bader requested review from elizabethandrews and Fznamznon September 11, 2020 08:52

Revert "[SYCL] Support or diagnose use of namespace std types as kern…

8d0087e

…el type names" This reverts commit 46c54c6. # Conflicts: # clang/test/CodeGenSYCL/stdtypes_kernel_type.cpp # clang/test/SemaSYCL/unnamed-kernel.cpp

jtmott-intel dismissed stale reviews from rbegam and premanandrao via 8d0087e September 11, 2020 16:56

elizabethandrews approved these changes Sep 11, 2020

View reviewed changes

rbegam approved these changes Sep 11, 2020

View reviewed changes

bader merged commit dd7fec8 into intel:sycl Sep 14, 2020

jtmott-intel deleted the remote_fwd_decl_namespace branch September 14, 2020 14:54

jtmott-intel mentioned this pull request Sep 14, 2020

[SYCL] compiler issues an error when std::nullptr_t is used as a kernel name type #247

Closed

jtmott-intel restored the remote_fwd_decl_namespace branch September 15, 2020 16:32

aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023

Apply changes from intel/llvm intel#1963

de8bed4

		Diag.Report(KernelLocation, diag::err_sycl_kernel_incorrectly_named)
		<< /* kernel name is missing */ 0;

[SYCL] Support or diagnose use of namespace std types as kernel type names #1963

[SYCL] Support or diagnose use of namespace std types as kernel type names #1963

Conversation

jtmott-intel commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtmott-intel Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichkeane commented Jun 24, 2020

premanandrao commented Jun 24, 2020

erichkeane commented Jun 24, 2020

jtmott-intel commented Jun 24, 2020

rolandschulz commented Jun 24, 2020

erichkeane commented Jun 24, 2020

rolandschulz commented Jun 25, 2020

jtmott-intel commented Jun 25, 2020

jtmott-intel commented Jul 2, 2020

bader commented Jul 15, 2020

jtmott-intel commented Jul 16, 2020

bader commented Jul 16, 2020

erichkeane commented Aug 18, 2020

jtmott-intel commented Aug 31, 2020

romanovvlad commented Sep 1, 2020

jtmott-intel commented Sep 1, 2020

romanovvlad commented Sep 2, 2020

jtmott-intel commented Sep 2, 2020

romanovvlad commented Sep 3, 2020

jtmott-intel commented Sep 5, 2020

premanandrao left a comment

Choose a reason for hiding this comment

jtmott-intel commented Sep 10, 2020 • edited Loading

rbegam left a comment

Choose a reason for hiding this comment

elizabethandrews left a comment

Choose a reason for hiding this comment

jtmott-intel commented Sep 12, 2020

elizabethandrews commented Sep 14, 2020

jtmott-intel Sep 10, 2020 •

edited

Loading

jtmott-intel commented Sep 10, 2020 •

edited

Loading