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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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

> > In D74387#1869845 <https://reviews.llvm.org/D74387#1869845>, @rjmccall wrote:
> > 
> >> The right approach here is probably what we do in ObjC ARC when we see types that are illegal in ARC: in system headers, we allow the code but add a special `UnavailableAttr` to the declaration so that it can't be directly used.
> >>
> >> That is straightforward enough that I think you should just do it instead of leaving this as technical debt.
> > 
> > 
> > I haven't considered something like this, because I'm not familiar with ObjC at all... I will give it a try, thanks.
>
> Hi @rjmccall , I assume, I took a look at this. 
>  Let's imagine, I will try to diagnose `__float128` type using already implemented functionality. It seems like I need to call something like
>
>   S.DelayedDiagnostics.add(                                        
>       sema::DelayedDiagnostic::makeForbiddenType(loc,              
>           diag::err_type_unsupported, type, "__float128"));
>   `
>
> right?
>  I suppose, then this diagnostic will be saved and emitted inside function named `handleDelayedForbiddenType`.
>  Here it checks that this forbidden type is actually allowed and emits a diagnostic if it's not.


This isn't quite right.  The goal here is to delay the diagnostic *twice*.  The first delay is between the point where we parse/process the type (i.e. SemaType) and the point where we've fully processed the declaration that the type is part of (i.e. SemaDecl).  That's the point where we call `handleDelayedForbiddenType`, and you're right that it's too early to know whether the declaration is really device code.  However, you're missing something important about how `handleDelayedForbiddenType` works: it's never really trying to suppress the diagnostic completely, but just to delay it for certain declarations until the point that the declaration is actually used, under the hope that in fact it will never be used and everything will work out.  For ARC, we chose to delay for all declarations in system headers, under the assumption that (1) system headers will never introduce functions that have to be emitted eagerly, and (2) we always want to warn people about problematic code in their own headers.  Those choices don't really fit SYCL's use case, and you should change the logic in `isForbiddenTypeAllowed` to delay your diagnostic for essentially all declarations (except kernels?), since effectively all non-kernel code in device mode is lazily emitted.  But if you do that, it should combine well with CUDA/OMP deferred diagnostics:

- 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`.
- 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`.

> It seems that the second problem is the same problem which prevented me from implementing diagnosing of `__float128` type through CUDA/OMP deferred diagnostics (I mentioned my attempt in the last comment https://reviews.llvm.org/D74387#1870014). I still need to find best place for diagnostic issuing. It seems that there are so many places where type can actually be introduced to resulting LLVM IR module, and in some of them I need to check some additional conditions to do not prevent `__float128` usage when it actually doesn't introduce forbidden type to resulting LLVM IR module.

The key thing here is that all uses should be associated with some top-level declaration that's either eagerly-emitted in device mode or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387