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

[Mariya Podchishchaeva via Phabricator via cfe-commits]

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:
> 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?


================
Comment at: clang/lib/Sema/SemaAvailability.cpp:479
+        case UnavailableAttr::IR_SYCLForbiddenType:
+          diag_available_here = diag::err_type_unsupported;
+          break;
----------------
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?


================
Comment at: clang/lib/Sema/SemaAvailability.cpp:534
+    if (S.getLangOpts().SYCLIsDevice)
+      S.SYCLDiagIfDeviceCode(Loc, diag) << ReferringDecl;
+    else
----------------
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.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:18030
+  if (LangOpts.SYCLIsDevice && FD->hasAttr<SYCLKernelAttr>())
+    return FunctionEmissionStatus::Emitted;
+
----------------
rjmccall wrote:
> So you want to emit it for the definition in addition to emitting it for specific specializations?
Somehow diagnostics are emitted only for the definitions. 
Without this change diagnostics aren't emitted at all.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7771
+    return true;
+  }
+
----------------
rjmccall wrote:
> I wonder if it's reasonable to treat all forbidden types the same here or if we want different functions for the ARC and SYCL use cases.
I think it could be reasonable if we will have forbidden type cases for SYCL sometime. For now, I don't see the purpose in a separate function for SYCL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387