Skip to content

[SYCL] Improve error diagnostic for invalid SYCL kernel name #4867

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 8, 2021

Conversation

smanna12
Copy link
Contributor

@smanna12 smanna12 commented Nov 1, 2021

According to SYCL 2020 spec:
The kernel name must be forward declarable at namespace scope (including global namespace scope) and may not be forward declared other than at namespace scope.

The use of nested struct-case (where the name IS globally visible but just not forward-declarable) in a SYCL kernel name currently generates invalid error message:

error: '{{.*}}' should be globally visible

This patch modifies the error diagnostic message when the kernel name is declared at non-namespace scope (i.e. Inside a function or class/struct).

Signed-off-by: Soumi Manna soumi.manna@intel.com

Signed-off-by: Soumi Manna <soumi.manna@intel.com>
@smanna12
Copy link
Contributor Author

smanna12 commented Nov 1, 2021

I do not think we can separate nested struct case used as kernel name and add a separate diagnostic for this.
I have updated current error message instead of using globally visible to make it generic for all cases and also match with current spec wording.

// Check if the kernel name is a Tag declaration
 // local to a non-namespace scope (i.e. Inside a function or within
 // another Tag etc).
if (!DeclCtx->isTranslationUnit() && !isa<NamespaceDecl>(DeclCtx)) {
        if (const auto *Tag = dyn_cast<TagDecl>(DeclNamed)) {
          // Check if the declaration is completely defined within a
          // function or class/struct.
          if (Tag->isCompleteDefinition()) {
            S.Diag(KernelInvocationFuncLoc,
                   diag::err_sycl_kernel_incorrectly_named)
                << /* non-forward-declarable type is invalid */ 0
                << KernelNameType;
            IsInvalid = true;
          } else {
            S.Diag(KernelInvocationFuncLoc, diag::warn_sycl_implicit_decl);
            S.Diag(DeclNamed->getLocation(), diag::note_previous_decl)
                << DeclNamed->getName();
          }
        }
      }
    

Please let me know if i missed something. Any suggestion if the updated diagnostic message is not clear or too long?

@smanna12 smanna12 marked this pull request as ready for review November 1, 2021 16:48
@smanna12 smanna12 changed the title [SYCL] Improve diagnostics for invalid SYCL kernel names [SYCL] Improve error diagnostic for invalid SYCL kernel name Nov 1, 2021
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
@smanna12 smanna12 requested a review from Fznamznon November 1, 2021 19:25
Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@smanna12
Copy link
Contributor Author

smanna12 commented Nov 2, 2021

Thanks for the reviews everyone.

@dm-vodopyanov dm-vodopyanov merged commit 455dce8 into intel:sycl Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants