[PATCH] D74387: [SYCL] Do not diagnose use of __float128

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D74387#1895264 <https://reviews.llvm.org/D74387#1895264>, @Fznamznon wrote:

> @rjmccall, Thank you very much for so detailed response, It really helps. I started working on implementation and I have a couple of questions/problems with this particular appoach.
>
> > - If `foo` uses `__float128` (whether in its signature or internally), that is invalid in device mode, but the diagnostic will be delayed by the forbidden-type mechanism, meaning that it will become an `unavailable` attribute on `foo`.
>
> So, for example if some variable is declared with `__float128` type, we are adding to parent function Unavaliable attribute, right?


That's how it's supposed to work.  I can't guarantee that it will actually always work that way, because I'm sure you'll be pushing on this code in some new ways.

>> - If `bar` uses `foo`, that use is invalid in device mode (because of the `unavailable` attribute), but the diagnostic will be delayed via the standard CUDA/OMP mechanism because we don't know yet whether `bar` should be emitted as a device function.
>> - If `kernel` uses `bar`, that will trigger the emission of the delayed diagnostics of `bar`, including the use-of-`unavailable`-function diagnostic where it uses `foo`.  It should be straightforward to specialize this diagnostic so that it reports the error by actually diagnosing the use of `__float128` at the original location (which is recorded in the `unavailable` attribute) and then just adding a note about how `foo` is used by `bar`.
> 
> Consider following example (this is absolutely valid SYCL code, except __float128 usage):
> 
>   // Host code:
>   __float128 A;
>   // Everything what lambda passed to `sycl_kernel` calls becomes device code. Capturing of host variables means that these variables will be passed to device by value, so using of A in this lambda is invalid.
>   sycl_kernel<class kernel_name>([=]() {auto B = A});
>    
>    
> 
> In this case we add unavailable attribute to parent function for variable `A` declaration. But this function is not called from device code. Please correct me if I'm wrong but it seems that we need to diagnose not only functions, but also  usages of any declarations with unavailable attribute including variable declarations, right?

Right.  The diagnosis side of that should already happen — `unavailable` diagnostics apply to uses of any kind of declaration, not just functions or variables.  Current delayed diagnostics should be enough to make the `unavailable` attribute get applied to the global  variable `A` in your example,  since it's a use from the declarator.   If SYCL supports C++-style dynamic global initializers, you'll probably need to add code so that uses of `__float128` within a global initializer get associated with the global, which currently won't happen because the initializer isn't "in scope".  But there are at least two other patches underway that are dealing with similar issues: https://reviews.llvm.org/D71227 and https://reviews.llvm.org/D70172.

> In addition, there are a couple of problems with this approach, for example with unevaluated `sizeof` context, i.e. code like this:
> 
>   sycl_kernel<class kernel_name>([=]() {int A = sizeof(__float128);});
> 
> 
> is diagnosed too, I think this is not correct.

Okay, that's a much thornier problem if you want to allow that.  What you're talking about is essentially the difference between an evaluated and an unevaluated context, but we don't generally track that for uses of *types*.  It's much easier to set things up so that you only complain about uses of *values* like the global variable A if they're in evaluated expressions, but types are never really "evaluated" in the same way that expressions are, so it's complicated.

I think that's a very separable question, so I would recommend you focus on the first problem right now, and then if you really care about allowing `sizeof(__float128)`, we can approach that later.

> I can upload what I have now to this review if it will help better (or maybe we will understand that I'm doing something wrong).
> 
> I'm also thinking about a bit another approach:
> 
> - If some declaration uses `__float128` it will become an unavailable attribute on this declaration. That means we don't always add unavailable attribute to the function which uses `__float128` internally.
> - In the place where clang actually emits `use-of-unavailable-declaration` diagnostics (somewhere in `DoEmitAvailabilityWarning` function, defined in `SemaAvailability.cpp`) for SYCL, we make these diagnostics deferred using CUDA/OMP deferred diagnostics mechanism (using SYCL-specific analog of function like `diagIfOpenMPDeviceCode`/`CUDADiagIfDeviceCode`).

Sure, but you'll have to write a custom walk of the body looking for uses of the type that you don't like; that seems like a lot of work to get right, and it'll tend to fail "open", i.e. allowing things you don't want to allow, whereas this approach will tend to fail "closed", i.e. tending towards being conservatively correct.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74387/new/

https://reviews.llvm.org/D74387