[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

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

> Re-implemented diagnostic itself, now only usages of declarations
>  with unsupported types are diagnosed.
>  Generalized approach between OpenMP and SYCL.


Great, thanks a lot!

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

> The tests are failing because calling function with unsupported type in arguments/return value is diagnosed as well, i.e. :
>
>   double math(float f, double d, long double ld) { ... } // `ld` is not used inside the `math` function
>   #pragma omp target map(r)
>     { r += math(f, d, ld); } // error: 'math' requires 128 bit size 'long double' type support, but device 'nvptx64-nvidia-cuda' does not support it
>
>
> Should we diagnose calls to such functions even if those arguments/return value aren't used?


Yes, please! The test case (which I added) is broken and would result in a crash when you actually ask for PTX and not IR: https://godbolt.org/z/vL5Biw
This is exactly what we need to diagnose :)

---

I think the code looks good and this looks like a really nice way to fix this properly.

I inlined some questions. We might need to add some test coverage (if we haven't already), e.g., for the memcpy case. For example in OpenMP an object `X` with such types should be ok in a `map(tofrom:X)` clause.



================
Comment at: clang/lib/Sema/Sema.cpp:1727
+
+  QualType Ty = D->getType();
+  auto CheckType = [&](QualType Ty) {
----------------
Nit: Move below `CheckType` to avoid shadowing and confusion with the arg there. 


================
Comment at: clang/test/OpenMP/nvptx_unsupported_type_codegen.cpp:21
   char c;
   T() : a(12), f(15) {}
   T &operator+(T &b) { f += b.a; return *this;}
----------------
Why is this not diagnosed? I mean we cannot assign 15 on the device, can we? Or does it work because it is a constant (and we basically just memcpy something)?

If it's the latter, do we have a test in the negative version that makes sure `T(int i) : a(i), f(i) {}` is flagged?


================
Comment at: clang/test/OpenMP/nvptx_unsupported_type_codegen.cpp:81
-// CHECK: define weak void @__omp_offloading_{{.+}}foo{{.+}}_l75([[BIGTYPE:.+]]*
-// CHECK: store [[BIGTYPE]] {{0xL00000000000000003FFF000000000000|0xM3FF00000000000000000000000000000}}, [[BIGTYPE]]* %
----------------
Just checking, we verify in the other test this would result in an error, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387