[PATCH] D74387: [SYCL] Defer __float128 type usage diagnostics

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon marked an inline comment as done.
Fznamznon added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:12248
+  ///   SYCLDiagIfDeviceCode(Loc, diag::err_type_unsupported) << "__float128";
+  DeviceDiagBuilder SYCLDiagIfDeviceCode(SourceLocation Loc, unsigned DiagID);
+
----------------
rjmccall wrote:
> Fznamznon wrote:
> > rjmccall wrote:
> > > Will this collect notes associated with the diagnostic correctly?
> > Could you please make your question a bit more concrete?
> > This function is supposed to work in the same way as `Sema::CUDADiagIfDeviceCode` and `Sema::diagIfOpenMPDeviceCode` . It emits given diagnostic if the current context is known as "device code" and makes this diagnostic deferred otherwise. It uses the `DeviceDiagBuilder` which was implemented earlier. This `DeviceDiagBuilder` also tries to emit callstack notes for the given diagnostics. Do you mean these callstack notes or something else?
> Logically, notes that are emitted after a warning or error are considered to be part of that diagnostic.  A custom `DiagBuilder` that only redirects the main diagnostic but allows the notes to still be emitted will effectively cause those notes to misleadingly follow whatever previous diagnostic might have been emitted.
> 
> I call this out specifically because some of the places where you're using this still seem to try to emit notes afterwards, at least in some cases.  It's possible that `CUDADiagIfDeviceCode` happens to not be used in such a way.  Really I'm not sure this conditional `DiagBuilder` approach was a good idea the first time, and I think we should probably reconsider rather than duplicating it.
I think if there are some notes associated with the main diagnostic and we want to make this diagnostic deferred by using `SYCLDiagIfDeviceCode`, we have to use this function `SYCLDiagIfDeviceCode` for notes as well. In my changes I didn't do so because I didn't expect notes emitted after new diagnostic.
In our SYCL implementation we find function like `SYCLDiagIfDeviceCode` pretty useful because we don't know where is device code until templates are instantiated. We need some mechanism to defer diagnostics pointing to unsupported features used in device code.
Do you have better approach in mind?


================
Comment at: clang/lib/Sema/SemaAvailability.cpp:479
+        case UnavailableAttr::IR_SYCLForbiddenType:
+          diag_available_here = diag::err_type_unsupported;
+          break;
----------------
rjmccall wrote:
> Fznamznon wrote:
> > rjmccall wrote:
> > > All of the other cases are setting this to a note, not an error, so I suspect this will read wrong.
> > Yes, this is not a note. For such samples:
> > 
> > ```
> > int main() {
> >   __float128 CapturedToDevice = 1;
> >   kernel<class variables>([=]() {
> >     decltype(CapturedToDevice) D;
> >   });
> > }
> > ```
> > It looks like this:
> > ```
> > float128.cpp:63:14: error: 'CapturedToDevice' is unavailable
> >     decltype(CapturedToDevice) D;
> >              ^
> > float128.cpp:59:14: error: '__float128' is not supported on this target   /// This emitted instead of note 
> >   __float128 CapturedToDevice = 1;
> >              ^
> > ```
> > I had feeling that it should probably be a note. But there is no implemented note for unsupported types. I think I can add a new one if it will make it better. Should I?
> Yeah, this should be a note, like "note: variable is unavailable because it uses a type '__float128' that is not supported on this target".  You should add that.
Okay, done.


================
Comment at: clang/lib/Sema/SemaAvailability.cpp:534
+    if (S.getLangOpts().SYCLIsDevice)
+      S.SYCLDiagIfDeviceCode(Loc, diag) << ReferringDecl;
+    else
----------------
rjmccall wrote:
> Fznamznon wrote:
> > rjmccall wrote:
> > > Are you sure you want to be applying this to all of the possible diagnostics here, rather than just for SYCLForbiddenType unavailable attributes?
> > I suppose it is reasonable if we want to reuse unavaliable attribute for other SYCL use cases. Plus, In SYCL we don't know where is device code until we instantiate templates, it happens late, so we have to defer any diagnostic while compiling for device, otherwise we can point to host code where much more is allowed.
> My point is actually the reverse of that.  This code path is also used for normal `unavailable` attributes, not just the special ones you're synthesizing.  Diagnostics from the use of explicitly-unavailable declarations shouldn't get any special treatment here, no more than you'd give special treatment to a diagnostic arising from an attempt to assign a pointer into a `float`.  In the logic above where you recognize `IR_SYCLForbiddenType`, I think you should just check whether you should transitively defer the diagnostic and, if so, do so and then bail out of this function early.  That might mean you don't need the custom DiagBuilder, too.
Okay, I understand. I was under impression that `unavailable` attributes can appear only for ObjC ARC, so It is safe to defer everything in SYCL, so I moved calls of `SYCLDiagIfDeviceCode` as you requested.
It's a bit unclear how to avoid custom `DiagBuilder` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387