[SYCL] Move variadic call diagnostics to delayed diagnostics, fixes. #998

erichkeane · 2020-01-10T16:19:27Z

One of our downstreams requires using the variadic call diagnostics from
SYCL, however the SemaSYCL AST walker ends up being messy. This patch
initially removes the check from the AST walker and instead implements
its in normal SEMA via the delayed diagnostics mechanism.

However, while evaluating this, we discovered that there are quite a few
issues with how the diagnostics are emitted, so this patch includes
quite a few cleanups to make sure the diagnostics are emitted properly.
This results in some additional notes in some of the test.

Additionally, the asm checks were previously implemented in BOTH delayed
diagnostics as well as the AST walker, since the bugs in each aligned to
have full coverage. This patch ends up removing it from the AST walking
code since the other bugs were fixed.

Signed-off-by: Erich Keane [email protected]

One of our downstreams requires using the variadic call diagnostics from SYCL, however the SemaSYCL AST walker ends up being messy. This patch initially removes the check from the AST walker and instead implements its in normal SEMA via the delayed diagnostics mechanism. However, while evaluating this, we discovered that there are quite a few issues with how the diagnostics are emitted, so this patch includes quite a few cleanups to make sure the diagnostics are emitted properly. This results in some additional notes in some of the test. Additionally, the asm checks were previously implemented in BOTH delayed diagnostics as well as the AST walker, since the bugs in each aligned to have full coverage. This patch ends up removing it from the AST walking code since the other bugs were fixed. Signed-off-by: Erich Keane <[email protected]>

premanandrao · 2020-01-10T18:16:07Z

clang/lib/Sema/SemaSYCL.cpp

-        << Sema::KernelUseAssembly;
-    return true;
-  }
-
  // The call graph for this translation unit.


Were these dead code and safe to remove?

They were not, but are now :) When This is the case in the commit message where we had done 2 versions of diagnostics for the inline assembly, but bugs in each made them not cover all cases, but combined, they seemed to. My fix to the delayed diagnostics made the assembly error handling in normal 'Sema' cover the cases that this caught.

clang/test/SemaSYCL/sycl-cconv.cpp

clang/lib/Sema/SemaOverload.cpp

clang/lib/Sema/SemaSYCL.cpp

Fznamznon · 2020-01-13T14:00:58Z

clang/lib/Sema/SemaSYCL.cpp

@@ -1420,18 +1407,20 @@ Sema::DeviceDiagBuilder Sema::SYCLDiagIfDeviceCode(SourceLocation Loc,
                                                   unsigned DiagID) {
  assert(getLangOpts().SYCLIsDevice &&
         "Should only be called during SYCL compilation");
-  DeviceDiagBuilder::Kind DiagKind = [this] {
-    if (isKnownEmitted(*this, dyn_cast<FunctionDecl>(CurContext)))
+  FunctionDecl *FD = dyn_cast<FunctionDecl>(CurContext);


Shouldn't it be CurLexicalContext?

I believe it SHOULD be getCurLexicalContext, but the current implementation was CurContext. I couldn't come up with a reproducer that I was able to justify changing the status quo here.

I should double check, because it was before long vacation, but in my patch I've implemented a new usage of deferred diagnostics - diagnosing of __float128 usage in device code (https://github.com/intel/llvm/pull/971/files#diff-643c0b5e3365e57856a6871266a81b35) and it didn't work before I changed CurContext to getCurLexicalContext here.

Neat! Thanks for providing justification. I'll upload an updated patch momentarily.

@premanandrao

@premanandrao saw that I had accidentially left a line in the test commented out while debugging. Revert the commented out line to still be testing what it is supposed to be testing. Signed-off-by: Erich Keane <[email protected]>

Fznamznon · 2020-01-14T11:19:20Z

I actually thought that we also will fix this FIXME

llvm/clang/lib/Sema/SemaType.cpp

Line 4688 in 8803f62

// FIXME: Use deferred diagnostics engine to skip host side issues.

with this change.

erichkeane · 2020-01-14T14:21:13Z

I actually thought that we also will fix this FIXME

llvm/clang/lib/Sema/SemaType.cpp

Line 4688 in 8803f62

// FIXME: Use deferred diagnostics engine to skip host side issues.

with this change.

I don't think so, do we? That is OpenCL diagnostics, which I didn't change. In fact, I'd be surprised if the OpenCL path for these diagnostic repairs don't have similar issues to what I fixed.

Fznamznon · 2020-01-14T14:28:17Z

I actually thought that we also will fix this FIXME

llvm/clang/lib/Sema/SemaType.cpp

Line 4688 in 8803f62

// FIXME: Use deferred diagnostics engine to skip host side issues.

with this change.

I don't think so, do we? That is OpenCL diagnostics, which I didn't change. In fact, I'd be surprised if the OpenCL path for these diagnostic repairs don't have similar issues to what I fixed.

@bader , do you have any thoughts?

bader · 2020-01-14T14:54:23Z

Variadic functions are not allowed in OpenCL and SYCL device code.
OpenCL uses exception and allows printf.
There is no such exception for SYCL in the spec. It says that printf functionality is covered by stream class.
According to my interpretation of the SYCL spec, printf is no different from any other variadic function and should be diagnosed in the SYCL device code.
From what I see, today compiler allows using printf in the device code w/o reporting any errors, which seems to be a bug mentioned in the FIXME comment.

That is OpenCL diagnostics, which I didn't change. In fact, I'd be surprised if the OpenCL path for these diagnostic repairs don't have similar issues to what I fixed.

@erichkeane, I'm not sure I understand: what "issues" do you refer here?

erichkeane · 2020-01-14T14:59:39Z

Variadic functions are not allowed in OpenCL and SYCL device code.
OpenCL uses exception and allows printf.
There is no such exception for SYCL in the spec. It says that printf functionality is covered by stream class.
According to my interpretation of the SYCL spec, printf is no different from any other variadic function and should be diagnosed in the SYCL device code.
From what I see, today compiler allows using printf in the device code w/o reporting any errors, which seems to be a bug mentioned in the FIXME comment.

That is OpenCL diagnostics, which I didn't change. In fact, I'd be surprised if the OpenCL path for these diagnostic repairs don't have similar issues to what I fixed.
@erichkeane, I'm not sure I understand: what "issues" do you refer here?

The ones I fixed in this patch :), particularly when it comes to templates. OpenCL uses a similar (though not identical) delayed diagnostics mechanism that I have not evaluated (since improving OpenCL is not my goal here) but look similar enough to the issues that the SYCL code has currently (at least until this patch is commited).

I believe that FIXME is NOT fixed, since it is diagnosing OpenCL (which this patch does not), and I believe that fixing it out of the scope of both this patch and this repo.

bader · 2020-01-14T16:17:58Z

OpenCL uses a similar (though not identical) delayed diagnostics

Really? OpenCL compiler work with "device code" only, so the reason to "delay" diagnostics might be quite different from the SYCL device compiler, if any. Could you point the source code where OpenCL delays the diagnostics, please?

I believe that FIXME is NOT fixed,

Right.

since it is diagnosing OpenCL (which this patch does not), and I believe that fixing it out of the scope of both this patch and this repo.

The FIXME comment is not about fixing OpenCL behavior, but rather fixing SYCL compiler behavior, which I described in my previous comment: #998 (comment). Having said that I think this patch is the right place to fix diagnostic for printf, which is "variadic call". Does it make sense?

erichkeane · 2020-01-14T16:27:02Z

OpenCL uses a similar (though not identical) delayed diagnostics

Really? OpenCL compiler work with "device code" only, so the reason to "delay" diagnostics might be quite different from the SYCL device compiler, if any. Could you point the source code where OpenCL delays the diagnostics, please?

Ah, I was thinking of CUDA. I see a bunch of delay-diagnostics code for CUDA. Don't know why I remembered 'OpenCL' :).

I believe that FIXME is NOT fixed,

Right.

since it is diagnosing OpenCL (which this patch does not), and I believe that fixing it out of the scope of both this patch and this repo.

The FIXME comment is not about fixing OpenCL behavior, but rather fixing SYCL compiler behavior, which I described in my previous comment: #998 (comment). Having said that I think this patch is the right place to fix diagnostic for printf, which is "variadic call". Does it make sense?

I guess I still don't get what you're trying to say. It seems that we explicitly exclude ourselves from that check, and my patch does NOT exclude printf, so it should be doing what you want, right?

As a side note, that OpenCL check doesn't work correctly in C++... it seems to exclude ANY function named printf, not just the one in stdio.h :)

bader · 2020-01-14T17:01:13Z

I guess I still don't get what you're trying to say. It seems that we explicitly exclude ourselves from that check, and my patch does NOT exclude printf, so it should be doing what you want, right?

It was excluded by initial implementation. Previous implementation just skipped the diagnostics to allow using printf on in the host part of the code (i.e. before we had any diagnostics for SYCL device code). I think what Marya proposes is to include the fix for this bug into this PR.

We either should:

just remove the comment as deferred diagnositcs for variadic function calls was implemented already. Could you make sure that we have a LIT tests covering calling printf from the device code?
add separate deferred check for the printf calls. Instead of skipping function type check in SYCL device mode, add a new if for SYCL and use deferred diagnostics engine to report the error at this place.

BTW, ideally we should make use of existing diagnostics implemented for CUDA instead of adding new one: https://github.com/intel/llvm/blob/sycl/clang/include/clang/Basic/DiagnosticSemaKinds.td#L7631. Can we make this CUDA-specific diagnostic generic and re-use for SYCL? I think we can re-use the code reporting this diagnostic.

As a side note, that OpenCL check doesn't work correctly in C++... it seems to exclude ANY function named printf, not just the one in stdio.h :)

Feel free to report this. Tagging @AnastasiaStulova.

erichkeane · 2020-01-14T17:36:49Z

I guess I still don't get what you're trying to say. It seems that we explicitly exclude ourselves from that check, and my patch does NOT exclude printf, so it should be doing what you want, right?

It was excluded by initial implementation. Previous implementation just skipped the diagnostics to allow using printf on in the host part of the code (i.e. before we had any diagnostics for SYCL device code). I think what Marya proposes is to include the fix for this bug into this PR.

We either should:
* just remove the comment as deferred diagnositcs for variadic function calls was implemented already. Could you make sure that we have a LIT tests covering calling `printf` from the device code?

Got it. I'll remove the comment. sycl-cconv.cpp already tests with printf.

* add separate deferred check for the `printf` calls. Instead of skipping function type check in SYCL device mode, add a new if for SYCL and use deferred diagnostics engine to report the error at this place.
BTW, ideally we should make use of existing diagnostics implemented for CUDA instead of adding new one: https://github.com/intel/llvm/blob/sycl/clang/include/clang/Basic/DiagnosticSemaKinds.td#L7631. Can we make this CUDA-specific diagnostic generic and re-use for SYCL? I think we can re-use the code reporting this diagnostic.

I can definitely switch to that diagnostic if you wish, but it isn't saying the same thing or diagnosing the same thing. That is diagnosing a function DECLARATION, not a function CALL. Thus, it is a different error diagnosis. Also note that I'm not adding a error message here.

As a side note, that OpenCL check doesn't work correctly in C++... it seems to exclude ANY function named printf, not just the one in stdio.h :)

Feel free to report this. Tagging @AnastasiaStulova.

@AnastasiaStulova : That likely needs to make sure that the printf is externC and in the global namespace.

@bader

As brought up by @bader in review. Signed-off-by: Erich Keane <[email protected]>

Fznamznon · 2020-01-15T08:28:18Z

I can definitely switch to that diagnostic if you wish, but it isn't saying the same thing or diagnosing the same thing. That is diagnosing a function DECLARATION, not a function CALL. Thus, it is a different error diagnosis. Also note that I'm not adding a error message here.

Yes, I'm not sure that we can reuse it because we should diagnose calls, not declarations. As far as I understand in CUDA in most cases device code is marked explicitly, meanwhile in SYCL any function can magically become a device function if called from a kernel.

Overall, maybe I like place for deferred diagnostic where OpenCL does diagnosing a bit more because IMO it simplifies searching through SEMA code, but if it works and it's already done it's ok too.

AnastasiaStulova · 2020-01-15T11:53:09Z

As a side note, that OpenCL check doesn't work correctly in C++... it seems to exclude ANY function named printf, not just the one in stdio.h

We don't have standard includes in OpenCL and also there are numerous ways to declare BIFs so the way to check is string compare the name. In the C++ mode it should indeed have C linkage.

erichkeane · 2020-01-15T13:10:21Z

Clang doesn't typically check where a function is defined/declared, so the string check is fine, just not specific enough. You need to make sure it is C linkage, not in a namespace, not a member function, etc. Note it could likely be a friend declaration (should likely be allowed), though I wonder about a friend definition...

AnastasiaStulova · 2020-01-15T15:31:00Z

Yes indeed this still needs to be done!

You need to make sure it is C linkage, not in a namespace, not a member function, etc. Note it could likely be a friend declaration (should likely be allowed), though I wonder about a friend definition...

Would some of these not be redundant? I would expect any C linkage functions wouldn't be a member function? In other words C linkage functions won't be parsed with C++ features i.e. class members, templates, etc...

erichkeane · 2020-01-15T15:49:16Z

Yes indeed this still needs to be done!

You need to make sure it is C linkage, not in a namespace, not a member function, etc. Note it could likely be a friend declaration (should likely be allowed), though I wonder about a friend definition...

Would some of these not be redundant? I would expect any C linkage functions wouldn't be a member function? In other words C linkage functions won't be parsed with C++ features i.e. class members, templates, etc...

Yes, some are. "C" linkage can't be applied to templates or class members, but still would apply to friend functions I would expect. Also, inside a namespace a function can still be extern "C".

AnastasiaStulova · 2020-01-15T16:04:00Z

Also, inside a namespace a function can still be extern "C".

I am not sure that should be diagnosed. At least for the namespaces it still doesn't seem to work as regular C++ functions would. There will only be one C linkage function with a certain name regardless of the namespace. Although if we want to be on the safe side we might add this to the check.

erichkeane · 2020-01-15T16:16:18Z

Also, inside a namespace a function can still be extern "C".

I am not sure that should be diagnosed. At least for the namespaces it still doesn't seem to work as regular C++ functions would. There will only be one C linkage function with a certain name regardless of the namespace. Although if we want to be on the safe side we might add this to the check.

I'm not an OpenCL language designer, so take my opinion as just that :) You're likely right that "extern C" is sufficient, as long as that is what you mean. Friend definitions can get strange I think, so just make sure that the decision is made having played with them. In reality, extern "C" is likely sufficient.

erichkeane requested review from bader, Fznamznon and premanandrao January 10, 2020 16:19

premanandrao reviewed Jan 10, 2020

View reviewed changes

clang/test/SemaSYCL/sycl-cconv.cpp Outdated Show resolved Hide resolved

Fznamznon reviewed Jan 13, 2020

View reviewed changes

erichkeane force-pushed the variadic_funcs_change branch from b7e674a to 626431c Compare January 13, 2020 14:57

romanovvlad assigned Fznamznon Jan 13, 2020

[SYCL] Remove FIXME comment that this invalidates.

04bfe60

As brought up by @bader in review. Signed-off-by: Erich Keane <[email protected]>

Fznamznon approved these changes Jan 15, 2020

View reviewed changes

bader approved these changes Jan 15, 2020

View reviewed changes

bader merged commit f537293 into intel:sycl Jan 15, 2020

erichkeane mentioned this pull request Jan 15, 2020

Delayed Diagnostics broken for some diagnostic types that don't check dependence. #1018

Open

Fznamznon mentioned this pull request Feb 26, 2020

[SYCL] Reject zero length arrays #1153

Merged

[SYCL] Move variadic call diagnostics to delayed diagnostics, fixes. #998

[SYCL] Move variadic call diagnostics to delayed diagnostics, fixes. #998

Uh oh!

Conversation

erichkeane commented Jan 10, 2020

Uh oh!

premanandrao Jan 10, 2020

Choose a reason for hiding this comment

Uh oh!

erichkeane Jan 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fznamznon Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

erichkeane Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

Fznamznon Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

Fznamznon commented Jan 14, 2020

Uh oh!

erichkeane commented Jan 14, 2020

Uh oh!

Fznamznon commented Jan 14, 2020

Uh oh!

bader commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane commented Jan 14, 2020

Uh oh!

bader commented Jan 14, 2020

Uh oh!

erichkeane commented Jan 14, 2020

Uh oh!

bader commented Jan 14, 2020

Uh oh!

erichkeane commented Jan 14, 2020 • edited by Fznamznon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fznamznon commented Jan 15, 2020

Uh oh!

AnastasiaStulova commented Jan 15, 2020

Uh oh!

erichkeane commented Jan 15, 2020

Uh oh!

AnastasiaStulova commented Jan 15, 2020

Uh oh!

erichkeane commented Jan 15, 2020

Uh oh!

AnastasiaStulova commented Jan 15, 2020

Uh oh!

erichkeane commented Jan 15, 2020

Uh oh!

Uh oh!

Fznamznon Jan 13, 2020 •

edited

Loading

bader commented Jan 14, 2020 •

edited

Loading

erichkeane commented Jan 14, 2020 •

edited by Fznamznon

Loading