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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
