WIP: [SYCL] Defer diagnostics about __float128 type usage #1040

Fznamznon · 2020-01-22T16:34:37Z

This pull request is created to gather SYCL working group internal feedback before uploading the patch to LLVM phabricator.

Reuse deferred diagnostic interface for SYCL.

Fznamznon · 2020-01-22T16:36:25Z

Please feel free to review title and description of commit too.

erichkeane · 2020-01-22T16:38:57Z

clang/include/clang/Sema/Sema.h

+  ///   immediately.
+  /// - If CurLexicalContext is a function and we are compiling
+  ///   for the device, but we don't know that this function will be codegen'ed
+  ///   for devive yet, creates a diagnostic which is emitted if and when we


s/devive/device

Done, thanks for the catch

erichkeane · 2020-01-22T16:39:13Z

clang/include/clang/Sema/Sema.h

+  /// Variables with thread storage duration are not allowed to be used in SYCL
+  /// device code
+  /// if (getLangOpts().SYCLIsDevice)
+  ///   SYCLDiagIfDeviceCode(Loc, diag::err_thread_unsupported);


the diag here should probably be a real one that SYCL rejects...

We'll likely implement this case too.

Switched to current diagnostic

erichkeane · 2020-01-22T16:41:31Z

clang/include/clang/Sema/Sema.h

+  ///   SYCLDiagIfDeviceCode(Loc, diag::err_thread_unsupported);
+  DeviceDiagBuilder SYCLDiagIfDeviceCode(SourceLocation Loc, unsigned DiagID);
+
+  /// Checks if Callee function is a device function and emits


This description isn't particularly clear to me. Is this for the call-graph based diagnostics here in SemaSYCL (rather than normal SEMA)? If so, I would think this should be going away as soon as we can make it so.

I actually saw this one earlier and have a hard time understanding the purpose of it.

Yeah, the description is a bit nasty, maybe I will rewrite it a bit...
Actually, this function is not about call-graph based diagnostics, it's for deferred diagnostics. Deferred diagnostics system also builds some callgraph, but it's not real CallGraph object, it's some DenseMap which helps to understand where device code sits.
CUDA and OpenMP also have functions like this (as far as I remember for CUDA it has name CheckCUDACall, but note that such functions has additional logic which is not needed for SYCL).

erichkeane · 2020-01-22T16:43:56Z

clang/lib/Sema/SemaExpr.cpp

@@ -267,6 +267,9 @@ bool Sema::DiagnoseUseOfDecl(NamedDecl *D, ArrayRef<SourceLocation> Locs,

    if (getLangOpts().CUDA && !CheckCUDACall(Loc, FD))
      return true;
+
+    if (getLangOpts().SYCLIsDevice)
+      checkSYCLDeviceFunction(Loc, FD);


Should this return 'true' in the error case like every other situation?

IMO, It should return something only if we will validate something inside. Like CUDA does validation of function calls. But for now we don't validate anything inside this function, we only check if we know that this function will be emitted or not.

erichkeane · 2020-01-22T16:44:24Z

clang/lib/Sema/SemaSYCL.cpp

+static bool isKnownEmitted(Sema &S, FunctionDecl *FD) {
+  assert(FD && "Given function may not be null.");
+
+  if (FD->hasAttr<SYCLKernelAttr>())


what about SYCLDeviceAttr?

SYCLDeviceAttr is not presented in LLVM master yet.

clang/lib/Sema/Sema.cpp

bader · 2020-01-22T16:50:24Z

clang/test/SemaSYCL/sycl-restrict.cpp

+// RUN: %clang_cc1 -triple spir64 -fsycl-is-device -verify -fsyntax-only %s
+
+void eh_ok(void)
+{


Please, clang-format-ify this test.

clang/include/clang/Sema/Sema.h

erichkeane · 2020-01-22T17:29:39Z

clang/test/SemaSYCL/sycl-restrict.cpp

+  // expected-error@+1 {{__float128 is not supported on this target}}
+  __float128 A;
+}
+


I have a couple more concerns with this... we would obviously like __float128 to be usable in unevaluated contexts, so can we test that? (Basically in a decltype/sizeof/etc). I'd also like to see some more indirection through template cases (at least eventually).

Okay, I've made some testing.
I see the following problems:

Using of __float128 is not diagnosed in function return value and parameters, but It should, because it add unsupported type to resulting IR i.e.

__float128 foo(__float128 P) { // is not diagnosed if this function is called from device code ... }

Using decltype with underlying __float128 is not diagnosed but It should, because it add unsupported type to resulting IR:

__float128 foo1() { // is not diagnosed if this function is called from device code ... } decltype(foo1()) D; // is not diagnosed if used in device code

Using of __float128 is diagnosed in sizeof operator, which, I suppose, shouldn't, because It doesn't adds unsupported type to resulting IR:

int E = sizeof(__float128); // diagnosed in device code

Is not diagnosed if used in classes:

template <class T> class Z { public: T field; __float128 field1; };

But if the last problem was expected by me, because we can't defer diagnostics which comes from non-function lexical contexts. Other ones were a surprise. Not sure where it comes from yet - problem with deferred diagnostics or place for diagnosing in Sema is wrong.

Okay, I've made some testing.
I see the following problems:

* Using of __float128 is not diagnosed in function return value and parameters, but It should, because it add unsupported type to resulting IR i.e.

__float128 foo(__float128 P) { // is not diagnosed if this function is called from device code ... }

Yeah, that is definitely a problem.

* Using decltype with underlying __float128 is not diagnosed but It should, because it add unsupported type to resulting IR:

__float128 foo1() { // is not diagnosed if this function is called from device code ... } decltype(foo1()) D; // is not diagnosed if used in device code

Both of those SHOULD diagnose, but you can do some stuff with decltype and metaprogramming that doesn't result in it making it to IR.

* Using of __float128 is diagnosed in sizeof operator, which, I suppose, shouldn't, because It doesn't adds unsupported type to resulting IR:

int E = sizeof(__float128); // diagnosed in device code

Yep, exactly. Though again, this isn't sizeof specific, decltype doesn't have to introduce the type either. Consider a case where it is a template parameter, but never otherwise instantiated:

template<typename t> void foo(){}; foo<__float128>();

Other than mangling, the float128 does nothing there.

* Is not diagnosed if used in classes:

template <class T> class Z { public: T field; __float128 field1; };

But if the last problem was expected by me, because we can't defer diagnostics which comes from non-function lexical contexts. Other ones were a surprise. Not sure where it comes from yet - problem with deferred diagnostics or place for diagnosing in Sema is wrong.

template<typename t> void foo(){}; foo<__float128>();

This is diagnosed. I guess, this is a problem too.
Thanks for the test case.

underlying __float128 is not diagnosed but It should, because it add unsupported type to resulting IR:
__float128 foo1() { // is not diagnosed if this function is called from device code
...
}
decltype(foo1()) D; // is not diagnosed if used in device code
Both of those SHOULD diagnose, but you can do some stuff with decltype and metaprogramming that doesn't result in it making it to IR.

Not sure that I understand. Could you please elaborate a bit more?

Decltype can be used in a bunch of places where it doesn't actually introduce an IR type, I think that is all I was getting at. Even something like the foo<__int128> case above is just a restatement of this case.

Consider something like:

__float128 nonemittedfunc(); std::conditional_t<SomeI < 1, decltype(nonemittedfunc()), int> SomeVar;

It is similar to the case, just saying that decltype doesn't necessarily mean that the type is introduced.

Okay, I extended the test and made some analysis, function parameters and return values is not diagnosed because in this case getCurLexicalContext() returns translation unit decl and we can diagnose only
inside function decls.
I was able to suppress diagnosing of sizeof expressions by adding a new check in SYCLDiagIfDeviceCode on whether we are in unevaluated context.
Didn't analyze decltype problems yet...

Fznamznon · 2020-02-03T13:28:21Z

I was thinking, shouldn't we only don't allow only operations with unsupported types? In other words diagnose only when some operation is performed with using of unsupported type?
For example we allow usage of classes with virtual methods inside the device code and diagnose only if virtual method is used.
Another example - OpenMP. There are also some unsupported types (including __float128), and diagnostic is emitted only if some operation is performed with using of this type (see 123ad19).
@bader , @erichkeane please provide your opinion on this.

erichkeane · 2020-02-03T13:58:28Z

I think it would be up to the SYCL standard. Something like that would make sense, since we only have a problem emitting float128, not doing compiletime operations on it. However, something that is SYCL_EXTERNAL with float128 return/params is emitting it.

Fznamznon · 2020-02-03T14:46:35Z

Actually, the SYCL spec says nothing about unsupported types usage. It says which types are supported, but it doesn't say what to do with unsupported types. Probably we need to clarify it...

Reuse deferred diagnostic interface for SYCL.

Do not diagnose usage of __float128 type in sizeof expressions

Fznamznon · 2020-02-07T12:52:49Z

I was able to handle more cases, but all these cases don't look sufficient to me, because adding types to IR happens in different places, I think I will never be able to handle all of them through Sema. For example auto variable declarations handled not the same way as regular variables, so I've made a some strange change after auto type deduction to diagnose if this type introduced __float128 type.
I suppose, we could try approach with diagnosing only operations, because in my opinion if there is no operations with type, there will be no problems, because OpenCL backends optimizers should be able to remove problematic type appearance as dead code, if it's not used.

keryell · 2020-02-07T14:10:00Z

Yes, I think we should not warn about problems which are not real problems...

erichkeane · 2020-02-07T14:10:59Z

I don't really have a good suggestion here unfortunately. The __float128 error handling is a little messy, and I presume there is going to be quite a few more examples that we can come up with.

An additional concern we have is that some of the SEMA checks that you added (particularly the auto-deduction one) is likely not going to work once we implement NTTPs and auto template params. I presume that we'll have to figure out what we're deducing for auto. It is a shame that there isn't a single 'create declaration' type place that we could diagnose this.

IMO the defered diagnostics work in this patch is worth while and would be a great addition to the llvm community. Is there any way to choose a less-difficult error message to attach to this patch instead? Something like 'inline assembly' or 'throw/try/catch' might be easier.

Fznamznon · 2020-02-07T14:21:50Z

Of course, I can prepare a patch which enables deferred diagnostics for SYCL with something easier than __float128, but we also need a solution to not emit errors about __float128 usage from standard headers, even it's not used in device code. Otherwise we can't compile any SYCL application which includes standard headers in host part (see #943 ). I thought that deferred diagnostics is better option for this problem, but I didn't think that it's so difficult to implement proper diagnosing for type usage.
I see only one option except deferred diagnostics usage: we can try just disable it for now, how it's done for CUDA and _Float16 type (https://reviews.llvm.org/D57369), until we figure out how to diagnose it properly.
Do you have any concerns/suggestions?

erichkeane · 2020-02-07T14:28:28Z

No, I'd consider just disabling it like CUDA does for now. We can add __Float128 disable to SemaSYCL at one point if that isn't sufficient until we can come up with a better way to do it.

Fznamznon · 2020-02-10T11:09:28Z

Okay. I'm only concerned about the fact that we committed this diagnostic to our branch. This diagnostic is not sufficient and If I make a change to llorg which just disables the diagnostic it will conflict with our branch.

Fznamznon · 2020-02-10T15:21:02Z

Okay, I prepared the patch for disabling the diagnostic #1109

intel#6164 adds an additional implicit argument to reduction kernels, so the XPTI test making assumptions about the number of arguments in a reduction kernel must have the assumptions updated.

Fznamznon added the upstream This change is related to upstreaming SYCL support to llorg. label Jan 22, 2020

Fznamznon requested review from bader, erichkeane and premanandrao January 22, 2020 16:34

erichkeane reviewed Jan 22, 2020

View reviewed changes

Fznamznon commented Jan 22, 2020

View reviewed changes

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

bader reviewed Jan 22, 2020

View reviewed changes

clang/include/clang/Sema/Sema.h Outdated Show resolved Hide resolved

erichkeane reviewed Jan 22, 2020

View reviewed changes

Fznamznon changed the title ~~[SYCL] Defer diagnostics about __float128 type usage~~ WIP: [SYCL] Defer diagnostics about __float128 type usage Jan 27, 2020

Fznamznon added 3 commits February 5, 2020 09:17

[SYCL] Defer diagnostics about __float128 type usage

b1460c0

Reuse deferred diagnostic interface for SYCL.

[SYCL] Extend the test, apply comments, fixes

bc84382

Do not diagnose usage of __float128 type in sizeof expressions

Implement more diagnostic cases

bfa18e0

Fznamznon force-pushed the float128-diag branch from 533e165 to bfa18e0 Compare February 7, 2020 12:45

Fznamznon requested review from erichkeane and bader February 7, 2020 12:54

Fznamznon closed this Feb 11, 2020

WIP: [SYCL] Defer diagnostics about __float128 type usage #1040

WIP: [SYCL] Defer diagnostics about __float128 type usage #1040

Uh oh!

Conversation

Fznamznon commented Jan 22, 2020

This pull request is created to gather SYCL working group internal feedback before uploading the patch to LLVM phabricator.

Uh oh!

Fznamznon commented Jan 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fznamznon Jan 22, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fznamznon commented Feb 3, 2020

Uh oh!

erichkeane commented Feb 3, 2020

Uh oh!

Fznamznon commented Feb 3, 2020

Uh oh!

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

Uh oh!

Uh oh!

keryell commented Feb 7, 2020

Uh oh!

erichkeane commented Feb 7, 2020

Uh oh!

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

Uh oh!

Uh oh!

erichkeane commented Feb 7, 2020

Uh oh!

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

Uh oh!

Uh oh!

Fznamznon commented Feb 10, 2020

Uh oh!

Uh oh!

Fznamznon Jan 22, 2020 •

edited

Loading

Fznamznon commented Feb 7, 2020 •

edited

Loading

Fznamznon commented Feb 7, 2020 •

edited

Loading

Fznamznon commented Feb 10, 2020 •

edited

Loading